1351427 - Retrofit mozIOSPreferences back to C++ interface (DateTimeFormat::FormatPRExplodedTime(), DateTimeFormat::FormatPRTime)

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement, P3)

Description

[Jorg K (CEST = GMT+2)]

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

As per bug 1342753 comment #47 it would be good if the C++ interface also used the OS settings to make the display consistent in certificate display and perhaps navigation history.

Comment 1

[Jorg K (CEST = GMT+2)]

Gregory, after your work on bug 1301640 and bug 1329841, maybe you'd be interested in this bug.

Comment 2

[Gregory Moore [:gmoore]]

Hi Jorg, thanks for asking. Unfortunately I am starting work really soon, so I don't think I will be able to work on this. Otherwise I would definitely be interested. Maybe this would be a good first or second bug for someone else though? Thanks again.

Comment 3

[Jorg K (CEST = GMT+2)]

Sadly, Gregory is not available for the bug, so Zibi and Masatoshi-san, could you give me a rough outline of what needs to be done here.

By the looks of it, the settings need to be retrieved from mozIntl.DateTimeFormat (how?) and stuck into ICU via a custom format like is already done here as a pattern:
https://dxr.mozilla.org/mozilla-central/rev/10ea10d9993c9701e5525928257a589dea2c05d8/intl/locale/DateTimeFormat.cpp#143

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

So, since you have Android separated out, I'd suggest you focus only on the non-Android variant.

The flow should be:

1) Match nsDateFormatSelector and nsTimeFormatSelector to OSPreferences::DateTimeFormatStyle
2) Once you have DateTimeFormatStyle for both date and time, you'll call OSPrefreences::ReadDateTimePattern
3) When you got the pattern you'll call udat_format with that pattern

Done.

The trick is that I'm not sure how many of the nsDateFormatSelector and nsTimeFormatSelector you can match to OSPreferences::DateTimeFormatStyles.
My guess would be that you'd want to match as many as you can, and the rest just keep as UDAT_*.

The end result should be actually much simpler than the current code tbh.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: WIP: 1351427-datetime.patch (v1).

This seems easy enough, so I made a start. OSPreferences::ReadDateTimePattern() wasn't public so I had to make it so.

So far I've only done it for dates.

Testing this in Thunderbird, the debug prints:
Short: yyyy-MM-dd
since TB uses short dates and that's what I've set the short date to in the OS.

However, there must be a missing bit or something not working since the display I get is still standard US: MM/dd/yyyy. Hmm.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: WIP 1351427-datetime.patch (v2).

(In reply to Jorg K (GMT+2) from comment #5)
> Testing this in Thunderbird, the debug prints:
> Short: yyyy-MM-dd
> since TB uses short dates and that's what I've set the short date to in the
> OS.
> However, there must be a missing bit or something not working since the
> display I get is still standard US: MM/dd/yyyy. Hmm.
Yes, that's because udatpg_getBestPattern() goes and ignores the original pattern and converts is into something more compatible with locale. It also adds a comma. Hmm. We can see this in the new patch.

Re. the new patch (v2): This is basically a gigantic cull. Since both dates and times are now patterns, all the code that was there to create time patterns from timeStyle = UDAT_MEDIUM/SHORT is basically gone. There is now one pattern only, composed out of date and time. Since the diff is really confusing, best apply the patch to see the finished result. I also fixed the many overlong lines.

I left the call to udatpg_getBestPattern() in so we can see its effect in the debug, however, I suggest to remove it in the final version since, as noted above, this call turns the pattern retrieved from the OS into something more compatible with the locale, which is exactly what we do *not* want, well, at least I don't.

Testing this in TB is fun: You change the OS setting and then hover the cursor over the rows in the thread pane and you immediately get the new format.

For example I see now |2015-02-09 17:16| matching |yyyy-MM-dd HH:mm| which is the OS preference.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8856102 [details] [diff] [review]
WIP 1351427-datetime.patch (v2).

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

I've noticed a small logic error, but I want to avoid more noise, so I'm not sending another patch without getting feedback on the first one.

::: intl/locale/DateTimeFormat.cpp
@@ +165,4 @@
>  
> +  if (skeletonTime.Length() != 0) {
> +    skeleton.AppendLiteral(" ");
> +    skeleton.Append(skeletonTime);

That logic is wrong, the blank should only be appended when both date and time are requested. I'll fix this in the next round.

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8856102 [details] [diff] [review]
WIP 1351427-datetime.patch (v2).

It looks good!

A few notes:

* Instead of exposing the Read* function, just use GetDateTimePatternForStyle or GetDateTimeSkeletonForStyle which are the public functions
* For the connector pattern for date/time combo use GetDateTimeConnectorPattern

Comment 9

[Jorg K (CEST = GMT+2)]

Zibi, thanks for the feedback. I'll address the issues. One question:
How do you feel about removing the udatpg_getBestPattern() call:

User asks for: yyyy-MM-dd
With call user gets MM/dd/yyyy on an en-US build
Without the call the user gets what he asked for.

What does the JS side do in this case? That's why I asked in bug 1339650 comment #33: How can I see the effect of OS changes in FF after mozIntl.DateTimeFormat has landed, which is regular ECMA402 DateTimeFormat *on steroids* ;-)

Comment 10

[Jorg K (CEST = GMT+2)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #8)
> GetDateTimePatternForStyle or GetDateTimeSkeletonForStyle which are the
> public functions
Sadly not so :-(

Comment 11

[Jorg K (CEST = GMT+2)]

This is all quite tricky. As example, I have yyyy-MM-dd configured in my OS.
ReadDateTimePattern() returns just that.
GetDateTimeSkeletonForStyle() returns yyMd
GetDateTimePatternForStyle() returns M/d/yy

So I guess the first one is right, since the skeleton vaguely represents what the operating system said, but I still have to work out how to turn this into a semi-decent pattern.

Comment 12

[Jorg K (CEST = GMT+2)]

OK, I've read up on skeletons and patterns. Looks like we always have to translate the skeleton to a pattern. But then I end up ignoring the OS settings:

This is what I have in Windows:
Date: yyyy-MM-dd
Time: HH:mm
And this is what I get:
=== Date Short: yyMd (via GetDateTimeSkeletonForStyle)
=== Time Short: hmm (via GetDateTimePatternForStyle)
=== Total format skeleton: |yyMd, hmm|
=== Locale pattern: |M/d/yy, h:mm a|

So I can't win. Maybe use udatpg_getPatternForSkeleton instead of udatpg_getBestPattern.

Sorry for rambling on, looks like I have to learn ICU here ;-(

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: WIP 1351427-datetime.patch (v3) - not working.

How about something like this using udatpg_getPatternForSkeleton() ?

Not working since I can't assign the returned UChar* back to my nsAutoString. Sigh, string processing in Mozilla, how many hours have I spent on this :-(

Comment 14

[Jorg K (CEST = GMT+2)]

I think the confusion here comes from mixing skeletons and patterns. There is a nice example here:
http://userguide.icu-project.org/formatparse/datetime

Locale  format pattern for skeleton “MMMMdjmm”  Example
en_US   "MMMM d 'at' h:mm a"                    April 2 at 5:00 PM
es_ES   "d 'de' MMMM, H:mm"                     2 de abril, 17:00

Starting with yyyy-MM-dd in Windows
ReadDateTimePattern() returns just that, yyyy-MM-dd, since this is already a pattern.
GetDateTimeSkeletonForStyle() returns yyMd
GetDateTimePatternForStyle() returns M/d/yy

So I think GetDateTimeSkeletonForStyle() is unusable since I don't want to retrieve a skeleton. GetDateTimePatternForStyle() basically ignores the OS pattern. So I think, ReadDateTimePattern() as I had it in v2 of the patch is the way to go. We read and obey the pattern from the OS.

We need special processing for the skeletons yyyyMM, yyyyMMMM, MMMM and EEE we use however, since they are skeletons that need to be translated to a pattern.

New patch coming up.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1351427-datetime.patch (v4) - working

OK, I think it's ready for a first review round.

I use ReadDateTimePattern() to read the OS's *pattern*. In case we have date skeletons yyyyMM, yyyyMMMM, MMMM and EEE, I translate them to patterns.

Times always have patterns that come from the OS.

I use GetDateTimeConnectorPattern() to joint date an time, and the comma is back ;-)

The resulting code reads very nicely, very linear:
- get date pattern/skeleton
- if skeleton, get pattern
- get time pattern
- join
- aTimeParameters processing
- final format
- ... and done ;-)
Sadly the patch looks like a mess, since basically all the code is reshuffled.

Comment 16

[Jorg K (CEST = GMT+2)]

This also fixes the bug that the date skeletons yyyyMM, yyyyMMMM, MMMM and EEE had a trailing space and got joined to the time using that space and without using the correct connector.

Masatoshi-san, can you please advise which tests need to be adapted now.

Comment 17

[Jorg K (CEST = GMT+2)]

Oh, I left some prints in the code which will of course be removed in the next round.

Comment 18

[Masatoshi Kimura [:emk]]

Comment on attachment 8856221 [details] [diff] [review]
1351427-datetime.patch (v4) - working

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

Why do you format date and time separately and connect them yourself?

Comment 19

[Jorg K (CEST = GMT+2)]

It's simper that way, isn't it? Or do you see a better option?

Also, for dates, I can start with a pattern or a skeleton. So I have to process the date skeleton separately first to get the date pattern and then join it to the time pattern. As pointed out in comment #16, the original code had a bug since it did *not* properly connect, for example EEE, to the time that followed. I got "Sun 17:24", but it should have been "Sun, 17:24".

BTW, I connect the patterns, not the formatted result.

Comment 20

[Masatoshi Kimura [:emk]]

IIUC, you are doing
1. ReadDateTimePattern(dateStyle, None, ...);
2. ReadDateTimePattern(None, timeStyle, ...);
3. GetDateTimeConnectorPattern();
4. Connect results on your own.

Why don't you just use ReadDateTimePattern(dateStyle, timeStyle, ...);? If it does not work well, isn't it a bug of ReadDateTimePattern?

(In reply to Jorg K (GMT+2) from comment #19)
> BTW, I connect the patterns, not the formatted result.

OK, please read it as "format date and time patterns".

Comment 21

[Jorg K (CEST = GMT+2)]

(In reply to Masatoshi Kimura [:emk] from comment #20)
> IIUC, you are doing
> 1. ReadDateTimePattern(dateStyle, None, ...);
> 2. ReadDateTimePattern(None, timeStyle, ...);
> 3. GetDateTimeConnectorPattern();
> 4. Connect both *pattern* results on your own.

Yes, that's one path through the code. Another one is:

1a. Assign fixed date *skeleton*, say, EEE.
2b. Convert that date skeleton to a date pattern.
2. ReadDateTimePattern(None, timeStyle, ...);
3. GetDateTimeConnectorPattern();
4. Connect result date and time patterns.

Yes, when I don't have a skeleton for date, your solution is less calls.

I think my solution here is more linear and simpler. You could special-case the "both date and time are done with patterns", but I still need my code for the date=skeleton and time=pattern case.

Anyway, I'll do a patch your way and then you can pick one ;-)

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: 1351427-datetime.patch (v5) - Alternative solution.

OK, here is the alternative solution.

If date and time both have patterns, only call ReadDateTimePattern() once.

The logic appears more convoluted, and no code is really saved. Downside, it also doesn't work, since ReadDateTimePattern() does *not* insert the locale specific connector, and I don't think it should, since it's merely reading the OS settings.

So I see now:
2017-04-07 16:16
and
Sun, 16:16
=== Total format pattern: |yyyy-MM-dd HH:mm|
=== Total format pattern: |ccc, HH:mm|

If you really think that this solution is better, we'd have to drill open ReadDateTimePattern() to make it obey the connector. I don't know which other code relies on the current behaviour.

v4 gave me:
2017-04-07, 16:16
and
Sun, 16:16.

So I'm still requesting review for v4 since I don't like v5:
- less linear logic
- more control variables
- no code saved
- doesn't work, and unclear how it should work.

Comment 23

[Jorg K (CEST = GMT+2)]

Oh, as Zibi pointed out, ReadDateTimePattern() doesn't work in Android, so we need to check the return values better and implement some fallback. But first we should decide which option we take.

Comment 24

[Jorg K (CEST = GMT+2)]

Oh, ReadDateTimePattern() is meant to read the connector:
http://searchfox.org/mozilla-central/rev/c4fdb67bca7889e59af9dd99c604651a49c4aafa/intl/locale/windows/OSPreferences_win.cpp#136
Well, it doesn't.

Comment 25

[Masatoshi Kimura [:emk]]

(In reply to Jorg K (GMT+2) from comment #24)
> Oh, ReadDateTimePattern() is meant to read the connector:
> http://searchfox.org/mozilla-central/rev/
> c4fdb67bca7889e59af9dd99c604651a49c4aafa/intl/locale/windows/
> OSPreferences_win.cpp#136
> Well, it doesn't.

OK, let's go with v4 for now. But could you at least file a bug about this?

Comment 26

[Zibi Braniecki [:zbraniecki][:gandalf]]

Oh, sorry Jorg, I lead you astray, the function you should use is `GetDateTimePattern[0]. I didn't think of C++ callers, so it's just a NSIMETHOD.
If you believe it would be better to have this as a regular C++ function, I'd prefer to expose the wrapper.

The reason I don't want touse ReadDateTimePattern here is that ReadDateTimePattern is "dummy", while "GetDateTimePattern" does all the fallbacking.


> User asks for: yyyy-MM-dd
> With call user gets MM/dd/yyyy on an en-US build
> Without the call the user gets what he asked for.
> What does the JS side do in this case?

Well, first of all users should ever ask for patterns. That's in my opinion a fundamental mistake in this API and that's why we decided not to expose pattern in ECMA402.

You can do one of the two things - you can assume that the user really wants a pattern he asks for - so he asked for "yyyy-MM-dd" and we should format this pattern irrelevant of his Regional Settings, and irrelevant of whether such pattern makes sense in his locale. (patterns are locale specific!)

Or, you can assume that the user did not really want the pattern, he just wanted a pattern *like* the one he asked for, but localized.
In this case you have to do the following loop:

1) Get skeleton for the pattern requested
2) Get localized pattern for the skeleton

But you still will not get the regional preferences for this call. The reason for that is that neither Windows nor MacOS does allow user to adjust the "yyyy-MM-dd" pattern (for the reasons I gave above). They only allow you to customize short/medium/long/full styles.
So when the user asks for "yyyy-MM-dd", we should just do one of the two things - find the localized version of the pattern, or use the provided pattern, and format it.

> How can I see the effect of OS changes in FF after mozIntl.DateTimeFormat has landed, which is regular ECMA402 DateTimeFormat *on steroids* ;-)

The easiest way right now is to open browser console and do sth like:

```
const mozIntl = Cc["@mozilla.org/mozintl;1"].getService(Ci.mozIMozIntl);
mozIntl.createDateTimeFormat(undefined, {timeStyle: "long"}).format(0);
```

and this should give you different time each time you call it depending on changes to hour12/24 in your regional preferences.

Notice: this will only do this, if your OS locale and Firefox/Thunderbird UI locale are the same language. For example - en-AU and en-US. Or de-AT and de-DE.

Both Windows and MacOS Internationalization HIG use the "Regional Settings" as "Regional Settings for the OS locale". So, if you have "de" Windows, you really only adjust the regional settings for "de-*", and when OSPrefrences request date/time patterns for "fr" or "en" locale, Windows and Mac return date/time patterns *without* those manual adjustments.

You can also see that when you change the language, Mac (and I think Windows too) reset your manual customizations to the defaults for the new locale. That's part of the HiG as well.

If you want to get "German date/time format customized by regional preferences but localized to en-US" then I'm afraid you're trying to work against Intl HiG.

[0] http://searchfox.org/mozilla-central/source/intl/locale/OSPreferences.cpp#344

Comment 27

[Jorg K (CEST = GMT+2)]

OK, more debugging, ReadDateTimePattern() on Windows retrieves connector |{1} {0}| without a comma via ReadDateTimePattern().

That's because in this code doesn't work:
GetDateTimeConnectorPattern(NS_ConvertUTF16toUTF8(localeName), aRetVal))
printf("=== locale %s\n", NS_ConvertUTF16toUTF8(localeName).get());
prints an empty string.

No idea what the
  WCHAR buffer[LOCALE_NAME_MAX_LENGTH];
  LPWSTR localeName = GetWindowsLocaleFor(aLocale, buffer);
  GetDateTimeConnectorPattern(NS_ConvertUTF16toUTF8(localeName), aRetVal)
is for, but it doesn't work.

Comment 28

[Jorg K (CEST = GMT+2)]

(In reply to Masatoshi Kimura [:emk] from comment #25)
> OK, let's go with v4 for now. But could you at least file a bug about this?
I'm about to fix it if you still prefer v5 ;-)

Addressing Zibi's comment #26 in a minute. Isn't it Sunday today :-(

Comment 29

[Zibi Braniecki [:zbraniecki][:gandalf]]

> OK, more debugging, ReadDateTimePattern() on Windows retrieves connector |{1} {0}| without a comma via ReadDateTimePattern().

Which is an appropriate connector. Read:
 - http://icu-project.org/apiref/icu4c/classicu_1_1DateTimePatternGenerator.html#afa4850437f49018ad74bf992619404f9
 - http://icu-project.org/apiref/icu4c/classicu_1_1DateTimePatternGenerator.html#a78e11a2ff03e72dfe4374318df5934da

Comment 30

[Jorg K (CEST = GMT+2)]

Maybe, but as per comment #27, ReadDateTimePattern() calls GetDateTimeConnectorPattern() with an empty string, although "en-US" was passed in and when calling GetDateTimeConnectorPattern() with "en-US" you get |{1}, {0}|. So there is a but here.

Comment 31

[Zibi Braniecki [:zbraniecki][:gandalf]]

> Maybe, but as per comment #27, ReadDateTimePattern() calls GetDateTimeConnectorPattern() with an empty string, although "en-US" was passed in and when calling GetDateTimeConnectorPattern() with "en-US" you get |{1}, {0}|. So there is a but here.

Interesting. Yeah, that may be a bug to file.

For example, http://searchfox.org/mozilla-central/source/intl/locale/windows/OSPreferences_win.cpp# may return empty string, instead of a negotiated locale.

Comment 32

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of Windows regional settings.

This is what an en-US version of Windows allows the user to chose: yyyy-MM-dd. So I'd say the user will then expect that to show up, no?

Of course localised versions of Windows will offer different *patterns* here, but once the user chose a pattern, that's what they want and not some other interpretation.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #26)
> Well, first of all users should [??] ever ask for patterns. That's in my opinion
> a fundamental mistake in this API and that's why we decided not to expose
> pattern in ECMA402.
"Not" missing where indicated?

> You can do one of the two things - you can assume that the user really wants
> a pattern he asks for - so he asked for "yyyy-MM-dd" and we should format
> this pattern irrelevant of his Regional Settings, and irrelevant of whether
> such pattern makes sense in his locale. (patterns are locale specific!)
Well, as I said, Windows offered that pattern for that particular version of the OS, so why ignore it?

Comment 33

[Zibi Braniecki [:zbraniecki][:gandalf]]

> This is what an en-US version of Windows allows the user to chose: yyyy-MM-dd. So I'd say the user will then expect that to show up, no?

Nope. This is a pattern for style - for "short date" it has "yyyy-MM-dd", but if you call FormatPRTime with "yyyy-MM-dd" there's no Windows settings to customize how to display that pattern.

> "Not" missing where indicated?

yep

> Well, as I said, Windows offered that pattern for that particular version of the OS, so why ignore it?

Windows offer a way to customize pattern for style, not pattern for pattern. It takes style on input, and gives pattern on output. The API you're reworking allows to take pattern on input.

Does it make sense?

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: 1351427-datetime.patch (v4b) - NOT working.

OK, before I try a different approach, let's see whether we can get something working first.

This is (v4) but trying to call GetDateTimePattern() which doesn't compile:
c:/mozilla-source/comm-central/mozilla/intl/locale/DateTimeFormat.cpp(96): error C3861: 'GetDateTimePattern': identifier not found

Frankly, I don't see why:
We have this
  AString getDateTimePattern(in long timeFormatStyle,
                             in long dateFormatStyle,
                             [optional] in ACString locale);
in the IDL and this in the implementation. What am I missing?
NS_IMETHODIMP
OSPreferences::GetDateTimePattern(int32_t aDateFormatStyle,
                                  int32_t aTimeFormatStyle,
                                  const nsACString& aLocale,
                                  nsAString& aRetVal)

As for comment #26:
1) Get skeleton for the pattern requested
2) Get localized pattern for the skeleton
I can try this, but I had the impression from my comment #12 (quoting blow) that this doesn't work:

This is what I have in Windows:
Date: yyyy-MM-dd
Time: HH:mm
And this is what I get:
=== Date Short: yyMd (via GetDateTimeSkeletonForStyle)
=== Time Short: hmm (via ???)
=== Total format skeleton: |yyMd, hmm|
=== Locale pattern: |M/d/yy, h:mm a|

So it steadfastly formats the months first and there is no way an en-US user can get the international format yyyy-MM-dd they really wanted.

Comment 35

[Masatoshi Kimura [:emk]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #33)
> Nope. This is a pattern for style - for "short date" it has "yyyy-MM-dd",
> but if you call FormatPRTime with "yyyy-MM-dd" there's no Windows settings
> to customize how to display that pattern.

Of course there isn't because FormatPRTime is our function and we can implement whatever we want. But it would be inconsistent with other apps because FormatPRTime is not the only consumer of the pattern string. Some consumers are not under our control.

Microsoft defines how the pattern string should be parsed.
https://msdn.microsoft.com/en-us/library/windows/desktop/dd317787(v=vs.85).aspx
Why is it better to ignore it and introduce the inconsistency with other apps?

Comment 36

[Zibi Braniecki [:zbraniecki][:gandalf]]

> Of course there isn't because FormatPRTime is our function and we can implement whatever we want.

What I'm saying is that there's no API in Windows that can get you the following operation:

pattern -> pattern altered by regional settings.

The only windows API you can get (and Mac) is:

style -> pattern altered by regional settings.


> Why is it better to ignore it and introduce the inconsistency with other apps?

I'm sorry but I don't understand that question. All I'm saying is that DateTimeFormat API (from Gecko) takes two *types* of input arguments:

 - style
 - pattern

For style input argument, we can go through OSPreferences, and get the pattern adjusted to use regional settings.

For pattern input argument, we cannot.

Does it make sense?

Comment 37

[Zibi Braniecki [:zbraniecki][:gandalf]]

> So it steadfastly formats the months first and there is no way an en-US user can get the international format yyyy-MM-dd they really wanted.

I believe it does work exactly the way you wanted. en-US does not have "yyyy-MM-dd" format. It's not an american custom.

So what you're doing is asking for the closest skeleton (skeletons are international) for "yyyy-MM-dd" and it gives you "yyMd" and then you ask for "en-US" pattern for this skeleton and it gives you "M/d/yy" which is the best en-US pattern for that skeleton.

If you want to get "yyyy-MM-dd" irrelevant of locale (what you called "international format") then you *don't* really want to do internationalization.
That's why I believe that this part of the API is plain wrong. It tries to do internationalization, but doesn't really want to do internationalization.

Comment 38

[Jorg K (CEST = GMT+2)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #31)
> > Maybe, but as per comment #27, ReadDateTimePattern() calls GetDateTimeConnectorPattern()
> > with an empty string, although "en-US" was passed in and when calling GetDateTimeConnectorPattern()
> > with "en-US" you get |{1}, {0}|. So there is a bug here.
> Interesting. Yeah, that may be a bug to file.
Filed bug 1354914 for that.

Comment 39

[Jorg K (CEST = GMT+2)]

Zibi, could you assist me in getting my patch compiled, see comment #34. What am I missing?

Comment 40

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ah! Sure :)
Tried to compile and only then noticed:

You're calling it as a function, while it's a method. Change to:

```
OSPreferences::GetInstance()->GetDateTimePattern
```

Comment 41

[Jorg K (CEST = GMT+2)]

Thanks, I feel like a dummy now :-( - Now it compiles, patch coming.

Comment 42

[Masatoshi Kimura [:emk]]

Attachment: dateformat.png

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #37)
> I believe it does work exactly the way you wanted. en-US does not have
> "yyyy-MM-dd" format. It's not an american custom.

Actually it has (at least in the Windows control panel). See the attached screenshot. ("英語 (米国)" means "English (US)" in Japanese.)

So "yyyy-MM-dd" is exactly the pattern altered by regional settings if the user choose it. How can we display this pattern altered by regional settings correctly? (obviously patch v4b does not.)

Comment 43

[Zibi Braniecki [:zbraniecki][:gandalf]]

> Actually it has (at least in the Windows control panel). See the attached screenshot. ("英語 (米国)" means "English (US)" in Japanese.)

What the screenshot shows is that Microsoft provided this pattern as one of the options for en-US "date-short".

This does not make it an american pattern, just a pattern you can select for the given style if you want.

> So "yyyy-MM-dd" is exactly the pattern altered by regional settings if the user choose it.

No, sorry, but not.

"yyyy-MM-dd" is an option for date-short style that the user can choose. That's very, very different from what you're saying.

On the most fundamental level - Windows gives it as a *value*, while intl/locale/DateTimeFormat.cpp takes it as *argument*.
On the other level, Windows does not let you customize the *value* for an *argument* "yyyy-MM-dd".

> How can we display this pattern altered by regional settings correctly?

My point is that there are no regional settings that would allow you to manipulate the result for this pattern.

If you want to display the pattern, just display it. Quite frankly, the recommended way of localizing "yyyy-MM-dd" would be to do sth like:

```
let year =  ...
let month = ...
let day = ...
let result = `${year}-${month}-${day}`;
```

because you already decided what pattern you want (so there's nothing to localize or customize), and now you just need the right numerals (western/eastern arabic). You don't even have any word to localize (like "January" or "Wednesday" here).

My point is - this is not internationalization.

Comment 44

[Jorg K (CEST = GMT+2)]

Attachment: 1351427-datetime.patch (v4c) - working again.

OK, this is almost the same as (v4) but now using GetDateTimePattern().

This is working and uses the OS *pattern* as a pattern, so users can get yyyy-MM-dd, which is an option Windows allows, although it's not strictly related to the en-US locale. If you look at what Windows offers for the en-US locale, you'll see may patterns which correspond to the locale, and some which don't.

Personally, I think the user should be able to chose and we shouldn't slap his wrist for choosing something that doesn't fit according to our rules.

This is certainly what the Thunderbird users want and since for FF it only shows in certificate details, it's not the end of the world if it doesn't fit 100% into Zibi's scheme which I haven't tried yet according to comment #26:
const mozIntl = Cc["@mozilla.org/mozintl;1"].getService(Ci.mozIMozIntl);
mozIntl.createDateTimeFormat(undefined, {timeStyle: "long"}).format(0);

Tests will still need adjustment.

Comment 45

[Zibi Braniecki [:zbraniecki][:gandalf]]

> it's not the end of the world if it doesn't fit 100% into Zibi's scheme

Agree :)

I like the patch, thanks a lot Jorg!

:emk, to make things clear (gosh, Intl is so complicated!)

You asked, how can we take "yyyy-MM-dd" customization. Jorg's patch does it - if the user asks for "date-long" and in Windows he selected "yyyy-MM-dd" for date-long, he'll get it!

My point was about another aspect of the DateTimeFormat.cpp API:

The API :Jorg is reworking, for example takes `kDateFormatYearMonthLong` as an argument which is "yyyyMMMM" skeleton.
there's no Windows or Mac or Linux API that would allow the user to customize the result pattern for this skeleton.

So I believe that we should just get the ICU pattern for this case, which is what the patch is doing :)

Hope that clears the confusion.

:Jorg - wrt Android there's a separate DateTimeFormat_android.cpp which handles the outlier.

Comment 46

[Masatoshi Kimura [:emk]]

Comment on attachment 8856255 [details] [diff] [review]
1351427-datetime.patch (v4c) - working again.

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

Honestly I'm not sure I fully understand the i18n here, but I believe Jorg and Zibi and I do not find obvious problems in the patch.

(In reply to Jorg K (GMT+2) from comment #44)
> Tests will still need adjustment.

You should update intl/locale/tests/gtest/TestDateTimeFormat.cpp so that the expected result matches the result from the new implementation.

Comment 47

[Jorg K (CEST = GMT+2)]

Attachment: 1351427-datetime.patch (v4d).

Final version. Removed print statements. Carrying forward Masatoshi's r+.

Comment 48

[Jorg K (CEST = GMT+2)]

(In reply to Masatoshi Kimura [:emk] from comment #46)
> You should update intl/locale/tests/gtest/TestDateTimeFormat.cpp so that the
> expected result matches the result from the new implementation.
Sure. Can someone please explain how to execute the test. I've been searching for documentation any only found this:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/GTest
That doesn't even give a single example of what someone should type to run the test. I don't really have time to read though mountains of documentation. It should be as easy as running a mochitest or xpcshell test from the command line.

Comment 49

[Zibi Braniecki [:zbraniecki][:gandalf]]

I only know how I execute my guests in intl/locale - 'mach gtest Intl*' - maybe you just need 'mach gtest TestDateTimeFormat*'? I think the match goes by test name not file name tho.

Comment 50

[Jorg K (CEST = GMT+2)]

Thanks, apparently you need to build with option |enable-tests|. Hmm. Looks like I need to build Firefox, which I haven't done in months.

Comment 51

[Jorg K (CEST = GMT+2)]

I'm compiling FF now. It may be very tricky to get those tests to pass. Before, the format went according to ICU/CLDR for the en-US locale, and now we're actually using what the OS has configured. That might be different from machine to machine, so if today the tests pass on a Windows 7 system, they might fail tomorrow on a Windows 10 system, not even to mention the other platforms which my have different date/time formats. We'll see.

try: -b o -p linux,linux64,macosx64,win32,win64 -u gtest
should be good for a try run, right? Or with Andriod:
try: -b o -p linux,linux64,macosx64,win32,android-api-15 -u gtest -t none ?

Comment 52

[Jorg K (CEST = GMT+2)]

OK, for the record, the test is run via |mach gtest DateTimeFormat*| since the .cpp file has |TEST(DateTimeFormat, ...|.

As predicted, it fails:
Expected: "January 1, 1970 at 12:00:00 AM"
Received: "01 January 1970, 00:00"
Expected: "01/1970, 12:00 AM"
Received: "01/1970, 00:00"

That's due to my system settings. Resetting my system to "English (United States)" I get these failures:

Expected: "January 1, 1970 at 12:00:00 AM"
Received: "Thursday, January 01, 1970, 12:00 AM"
Expected: "01/1970, 12:00:00 AM"
Received: "01/1970, 12:00 AM"

So the long date on Windows seems to contain the weekday, and there isn't a "medium" time format, so I think I'll have to switch this to "long" to get the seconds.

New patch coming.

Comment 53

[Jorg K (CEST = GMT+2)]

Attachment: 1351427-datetime.patch (v6).

OK, here is a code tweak (time medium->long) and the test changes. Since (v5) was used before, let's jump to (v6) here.

Comment 54

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02b35aa73fa1687293314320266c18352b6586ac

This will most likely fail on all platforms but Windows due to varying formats.

Comment 55

[Jorg K (CEST = GMT+2)]

As expected: Linux failure:
Expected: "Thursday, January 01, 1970, 12:00:00 AM"
Received:           "January 1, 1970, 12:00:00 AM GMT"
Expected: "01/1970, 12:00:00 AM"
Received: "01/1970, 12:00:00 AM GMT"

So looks like Linux uses a format with the time zone appended, no weekday, single digit day, pretty much like the original test only with the time zone added.

Sadly, the gtest seems to stop on the first failure, so we could be here for a while until we get all the test to pass.

Is there a way to check for various strings or should we test the platform and then compare with the platform-specific string?

Comment 56

[Jorg K (CEST = GMT+2)]

Mac also failed with the same problem. So summarising:

Long date and long time defaults:
Windows: Thursday, January 01, 1970, 12:00:00 AM
Linux/Mac:          January 1, 1970, 12:00:00 AM GMT

So what do we do?
- run the test on Mac/Linux only?
- accept two results?
- test the platform?

I'm open to suggestions.

Comment 57

[Zibi Braniecki [:zbraniecki][:gandalf]]

In my opinion Intl tests make little sense.

Tests like TestDateTimeFormat.cpp even less because they try to exact test against "best effort" approach of Intl formatting. In other words, there's not only the problem that the only thing you can compare a result of a call to is a result of the exact same operation (take date on the machine, take locale, take regional preferences, format it all together), but also that CLDR can over time *improve* how it displays date/time.

So, trying to cover that in a traditional unit test makes little sense. What if the test env. is not in English? Will we have to chase all tests and fix them if the way we format long date changes for en-US in CLDR 34?

I believe that you can do one of two things:

 - make the test only test no-errors and returning non-empty string (so, we retrieved *some* date or datetime)
 - make partial tests like, if you know the timezone, locale, time, you can test if the result contains "53" for minutes, or "January" for month name.

Comment 58

[Jorg K (CEST = GMT+2)]

Attachment: 1351427-datetime.patch (v7).

How about massaging the times a little in the test so they match?
Change double-digit day to single-digit, remove leading weekday and trailing GMT.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #57)
> What if the test env. is not in English?
That depends on whether the other locale has the same date/time patterns. When the extra-terrestrials take over the world, they'll have to go and fix this test ;-) 

Another try run for Linux and Mac since Windows passes locally:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0fec3f72c823e374244478ec767eabdf5c5209f

Comment 59

[Zibi Braniecki [:zbraniecki][:gandalf]]

> That depends on whether the other locale has the same date/time patterns. When the extra-terrestrials take over the world, they'll have to go and fix this test ;-)

That's not just extra-terrestials, arabic env will have easter-arabic numerals, some locales will have 24h clock, others 12h. German env will have different dayPeriod than en-US and so on.

But do the minimum you can.  I'll probably rewrite all tests in intl/locale at some point to be locale independent anyway and then I'll remove all those manual tests :)

Comment 60

[Jorg K (CEST = GMT+2)]

For a while to come, Mozilla will be in the US, and the tests will run on en-US servers.

Comment 61

[Zibi Braniecki [:zbraniecki][:gandalf]]

> For a while to come, Mozilla will be in the US, and the tests will run on en-US servers.

Well, that's a vicious circle. One of the reasons we can't run tests on non en-US servers is because all the tests that assume that it's on en-US ;)
I'd like to change that, but that's on me.

Comment 62

[Jorg K (CEST = GMT+2)]

Attachment: 1351427-datetime.patch (v7b).

Sorry about the noise, that basically worked, but I had forgotten to add |nt()| to all the relevant places.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6d1b76d3ab7d7df740c2e39997b96d04613b434

Comment 63

[Jorg K (CEST = GMT+2)]

Thanks for the review, but I'm having second thoughts now. They were prompted by comment #26:

===
The easiest way right now is to open browser console and do sth like:
const mozIntl = Cc["@mozilla.org/mozintl;1"].getService(Ci.mozIMozIntl);
mozIntl.createDateTimeFormat(undefined, {timeStyle: "long"}).format(0);
and this should give you different time each time you call it depending on changes to hour12/24 in your regional preferences.
Notice: this will only do this, if your OS locale and Firefox/Thunderbird UI locale are the same language. For example - en-AU and en-US. Or de-AT and de-DE.
===

The important part is here:
 this will only do this, if your OS locale and Firefox/Thunderbird UI locale are the same language.

I think we need to do something like this for the C++ implementation as well.

We should only used the OS defined pattern if the language set in the regional settings matches the language of the application. Here's why:

My development version of Thunderbird is en-US. I can set the region to Germany in Windows and select dd.MMM.yyyy as the pattern. This pattern is now used resulting in, for example: 15.May.2016.
That's just plain terrible. You have a German date format in en-US software and therefore English abbreviations.

Here's what I think we should do:
Get the OS locale via this code:
https://hg.mozilla.org/mozilla-central/rev/439f5e4ed073#l1.33

Compare the language of the OS locale with the language of the app locale.
Zibi: How do I do that? I'm sure you have something in your toolbox.

If the languages match, use the OS pattern. So a user with an en-US version of FF/TB can use any OS pattern any of the English locales offer, including yyyy-MM-dd, which is a "neutral" format some people prefer.

If the languages don't match, fall back to the previous behaviour where the OS pattern is not honoured and instead the default pattern for the app locale is used.

The code won't be much more complicated. We do the test and then call either GetDateTimePattern() for the OS pattern or GetDateTimePatternForStyle().

What do you think?

Comment 64

[Zibi Braniecki [:zbraniecki][:gandalf]]

> Compare the language of the OS locale with the language of the app locale.
Zibi: How do I do that? I'm sure you have something in your toolbox.

Sure I do!

That's what we do here: http://searchfox.org/mozilla-central/source/intl/locale/windows/OSPreferences_win.cpp#88

It should actually work on its own - if the languages don't match (languages being the first part of a locale, like en-AU and en-US will match) we'll give you a plain ICU date/time for your locale.

If they do, we'll use the OS regional settings.

Comment 65

[Jorg K (CEST = GMT+2)]

Thanks for the quick reply. Let me repeat my initial question:

Do you think that it's a good idea that the C++ interface should behave a bit more like the JS intereface in that OS settings/patterns are only used by an app which patches the language of those settings. Well, I think your answer would be "yes".

Now I'm a bit confused:

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #64)
> It should actually work on its own - if the languages don't match (languages
> being the first part of a locale, like en-AU and en-US will match) we'll
> give you a plain ICU date/time for your locale.
> If they do, we'll use the OS regional settings.

Are you saying that the call I'm using, GetDateTimePattern() will only return the pattern from the OS with the languages match and if languages don't match, give back the plain ICU formats? So my current code would already do what I think we should do?

Well, I can create a 06.Dec.2014 mix of German pattern (having switched to German) and English abbreviation (since in German it's Dez.). So is there a bug or a misunderstanding?

Comment 66

[Zibi Braniecki [:zbraniecki][:gandalf]]

>  Well, I think your answer would be "yes".

Yep :)

> Well, I can create a 06.Dec.2014 mix of German pattern (having switched to German) and English abbreviation (since in German it's Dez.). So is there a bug or a misunderstanding?

Not sure. It may be bug 1337067 for example. We unfortunately use a fairly aged Windows API to retrieve the OS locale, so there's a chance that you may have your Windows account in "de" but our API reads the "global" Windows language which may still be "en-US".

Please, check the 
```
Cc["@mozilla.org/intl/ospreferences;1"].
             getService(Ci.mozIOSPreferences).systemLocale;
```

If it returns "de", then you should not have your regional settings be reflected in mozIntl.DateTimeFormat in "en-US" Firefox.
If it returns "en-*", then we will match.

That's an interesting edge case since most users will not be affected (since they don't change Windows language all that often), but an engineer like you, who's testing changing languages, will be. :(

Anyway, yes, what I'm saying is that we do compare your OS language to your UI language and apply regional settings only if they match.

Comment 67

[Jorg K (CEST = GMT+2)]

Cc["@mozilla.org/intl/ospreferences;1"].
             getService(Ci.mozIOSPreferences).systemLocale;
returns "en-GB" for me since I have the Windows *system locale* set to "English (United Kingdom)".

In Windows 7 that's in "Region and Language", Administrative, "Change system locale...".

We know that that's the wrong locale to use. You want to use the "format locale" on "Region and Language", Formats. That locale defines which language patterns are offered.

That locale is retrieved with the code removed here:
https://hg.mozilla.org/mozilla-central/rev/439f5e4ed073#l1.33.
I haven't looked how you retrieve your system locale.

So since I have the system locale set to en-GB and that matches the app locale en-US, I can use any "format locale" and any pattern, even German, and it is *not* ignored. That's a bug and you're using the wrong interface to query the locale.

I've already discussed that at length in bug 1301640 comment #144.

Comment 68

[Zibi Braniecki [:zbraniecki][:gandalf]]

> That's a bug and you're using the wrong interface to query the locale.

Correct. The idea that you have a different locale your OS is in, and different locale your regional settings are formatted to, is a very specific Windows UX paradigm.
Neither MacOS nor Android or Linux have it.

I believe we should accommodate for it by extending OSPreferences to have a new method: OSPreferences::GetRegionalSettingsLocale which for all OSes except of Windows returns the same value as GetSystemLocale, while for Windows takes the locale you pointed out.

Then in OSPreferences::GetDateTimeFormat we should use this new method to retrieve locale we compare to UI locale.

I believe it's part of bug 1337067 since it's the bug where we'll have to improve the Windows API to retrieve the right locales.

Comment 69

[Zibi Braniecki [:zbraniecki][:gandalf]]

Actually, since it's all local to OSPreferences on Windows only, we could skip altering the public OSPreferences API and just add a function local to OSPreferences_windows.cpp to retrieve the RegionalSettingsLocale and use it in ReadDateTimeFormat.

Comment 70

[Jorg K (CEST = GMT+2)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #66)
> Anyway, yes, what I'm saying is that we do compare your OS language to your
> UI language and apply regional settings only if they match.
Where is that code? GetDateTimePattern() calls ReadDateTimePattern() and if that returns false, it calls GetDateTimePatternForStyle() instead. But at least for Windows, ReadDateTimePattern() always seems to return true, except when GetLocaleInfoEx() fails.

If what you say is right, I should be able to extend the test to formatting in de-DE and since that doesn't match en-* I should get the default ICU formatting for de with all OS patterns applied to en-* ignored. I'll try this later.

Comment 71

[Zibi Braniecki [:zbraniecki][:gandalf]]

> Where is that code?

http://searchfox.org/mozilla-central/source/intl/locale/windows/OSPreferences_win.cpp#88

If LanguagesMatch return true, we use the LOCALE_NAME_USER_DEFAULT variable on GetLocaleInfoEx which, in turn, returns the patterns altered by the regional settings.

If LanguagesMatch returns false, we use the App locale on GetLocaleInfoEx which will return a pattern for your UI locale not affected by the regional settings.

I don't think you'll get an ICU pattern, you'll get Windows pattern. (which as of today is different from ICU). You'll only get the ICU pattern if the ReadDateTimePattern will return some weird error.

Comment 72

[Jorg K (CEST = GMT+2)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #69)
> Actually, since it's all local to OSPreferences on Windows only, we could
> skip altering the public OSPreferences API and just add a function local to
> OSPreferences_windows.cpp to retrieve the RegionalSettingsLocale and use it
> in ReadDateTimeFormat.
Yes. I asked more questions in bug 1354914. I'm not sure why you want to use the system locale at all. This code
https://hg.mozilla.org/mozilla-central/rev/439f5e4ed073#l1.33.
retrieved the user/format locale. See nsILocaleService.idl for getSystemLocale() and getApplicationLocale().

BTW, "regional settings locale" is not a good name, since they are all "regional settings". I like "format locale", but Windows *may* call it user locale:
https://msdn.microsoft.com/en-us/library/windows/desktop/dd373815(v=vs.85).aspx

Comment 73

[Zibi Braniecki [:zbraniecki][:gandalf]]

> I'm not sure why you want to use the system locale at all.

Wait, let's separate two concepts. There's something called System Locale, and OSPreferences::GetSystemLocale should return it. It's the locale the OS is in.

Now, what we want here is the locale the regional settings are for, which in MacOS and Android is the same, but in Windows it's different.

I want to use it in Windows :)

> BTW, "regional settings locale" is not a good name, since they are all "regional settings". I like "format locale"

But there's one locale all those regional settings you format in, right?

OSPreferences::GetFormatLocale sounds good as well.

Comment 74

[Zibi Braniecki [:zbraniecki][:gandalf]]

> This code
https://hg.mozilla.org/mozilla-central/rev/439f5e4ed073#l1.33.
retrieved the user/format locale. See nsILocaleService.idl for getSystemLocale() and getApplicationLocale().

Ahh, I see.

Yeah, I'm totally fine with us comparing UI locale to the "User Locale" as in here: http://searchfox.org/mozilla-central/source/intl/locale/nsLocaleService.cpp#110

rather than to OS Locale as in here: http://searchfox.org/mozilla-central/source/intl/locale/windows/OSPreferences_win.cpp#21

Do you want to add it to this patch? :)

Comment 75

[Jorg K (CEST = GMT+2)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #71)
As I pointed out, there is a logic error somewhere. App locale (en-US) and system locale (en-GB) match and LOCALE_NAME_USER_DEFAULT (user locale: de-DE) is returned and its pattern is used.

So you're saying that if you fix that bug, the app locale is en-US and the user has set de-DE (ignoring system altogether), you'd then go to Windows to ask for the pattern for en-US. An option would be to return false on non-match and then use ICU via GetDateTimePatternForStyle(). 

> I don't think you'll get an ICU pattern, you'll get Windows pattern.
Right.

> You'll only get the ICU pattern if the
> ReadDateTimePattern will return some weird error.
Right.

It's very late I'll have to think about it more later.

Comment 76

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ok, I'll take the Windows ReadDateTimePattern patch to use User locale today!

Comment 77

[Zibi Braniecki [:zbraniecki][:gandalf]]

:Jorg, I believe you can land this patch without waiting for bug 1355308 - since the latter will just make it work better in the scenario where System OS locale and User OS locale is different.

Comment 78

[Jorg K (CEST = GMT+2)]

I think we have a problem here which has been discussed previously.

Masatoshi suggested in comment #20 to call ReadDateTimePattern() only once instead of processing date and time separately and joining them with the retrieved connector (whose get-interface I had to expose).

For this, I created (v5) which was later abandoned. However, as discussions in bug 1354914 and bug 1355308 show, the connector used should be based on the locale that is used for the formatting, say en-GB chosen in the OS, even of the app locale is en-US.

So the only way to get the correct connector is to call ReadDateTimePattern() once. If we do that, we now depend on bug 1355308. I need to have that fixed first, in fact I was going to include the fix here. Due to bug 1354914/bug 1355308, ReadDateTimePattern() gets an empty connector and that's not what the tests expect.

So to do it properly, we should first land bug 1355308.

In this bug here, I need to go back to (v5), of course adding the test changes, etc.

There is however a problem. In the example I have app=en-US, OS=en-GB.

Say the date/time requested is skeleton "EEEE" and short time. Currently I convert the skeleton into a pattern using en-US, I retrieve the connector with en-US, but I format the time with en-GB deep down in ReadDateTimePattern().

Is that acceptable? Maybe so, and in the future, we may introduce another function, to retrieve the exact format locale.

Comment 79

[Jorg K (CEST = GMT+2)]

There is another option:

Land this patch as is. It works but there is a potential inconsistency since GetDateTimePattern()/ReadDateTimePattern() will use the OS format locale and the caller will use the app locale for the skeleton->pattern and the connector.

Later we revisit this and create a function OSPreferences::GetRegionalSettingsLocale/GetEffectiveFormatLocale/or some such.

That function would return the locale passed in unless on Windows a language-matching format/user locale is found. So:
The new function returns en-US when called on a system with user/format locale de-DE but it returns en-GB on a system with user/format locale en-GB.

The C++ code would call this function first and then pass the returned locale to all further calls:
- skeleton to pattern
- get connector
- GetDateTimePattern()/ReadDateTimePattern().

That's perhaps the cleaner solution.

What do you think?

Comment 80

[Jorg K (CEST = GMT+2)]

Please advise, so I can dig out (v5) again according to comment #78 or land it according to comment #79. Are you available on IRC? In which time zone are you?

Comment 81

[Zibi Braniecki [:zbraniecki][:gandalf]]

I believe we should land this as-is, and work on the connector in bug 1354914 (low severity, low impact, just get a clean nice neat solution :)).

I'm on IRC on #developers, #l10n, #l20n,  #fx-team, etc. as :gandalf.

I'm in PST

Comment 82

[Jorg K (CEST = GMT+2)]

Bug 1354914 is not the right bug for this. That was about returning LOCALE_NAME_USER_DEFAULT which appears to be empty and then you retrieve the wrong connector. You're fixing this in bug 1355308 already.

As I have explained in comment #79, we need a OSPreferences::GetFOrmatLocale() to predict the locale that the OS will really use. I've filed bug 1355467 for that.

I really also wanted to add tests here that will request formatting for de-DE on an en-US machine to prove that the request is really ignored. I've filed bug 1355465 for that.

Comment 83

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #82)

Rephrasing this:
> I really also wanted to add tests here that will request formatting for
> de-DE on an en-US machine to prove that the request really retrieve the de-DE OS pattern
> and not any en-US pattern. I've filed bug 1355465 for that.

Comment 84

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72858bb2ed9d
Use OSPreferences for C++ date/time formats. r=emk

Comment 85

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for leaks detected by Linux x64 asan:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4c71612bdc8715b4949c4a6861deb656981bdb

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=72858bb2ed9d3882e12794cc3fe7091f6d68e21f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=90560366&repo=mozilla-inbound
[task 2017-04-11T18:10:16.362950Z] 18:10:16     INFO - TEST-INFO | LeakSanitizer | This can be done in testing/mozbase/mozrunner/mozrunner/utils.py
[task 2017-04-11T18:10:16.364945Z] 18:10:16    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at CountingMalloc, ICUReporter::Alloc, initHashtable, icu_58::DateTimePatternGenerator::addCLDRData
[task 2017-04-11T18:10:16.367269Z] 18:10:16    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at CountingMalloc, ICUReporter::Alloc, icu_58::DateTimePatternGenerator::DateTimePatternGenerator, icu_58::DateTimePatternGenerator::createInstance
[task 2017-04-11T18:10:16.369779Z] 18:10:16    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at CountingMalloc, ICUReporter::Alloc, _uhash_allocate, _uhash_init
[task 2017-04-11T18:10:16.372093Z] 18:10:16    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at CountingMalloc, ICUReporter::Alloc, icu_58::DateTimePatternGenerator::createInstance, udatpg_open_58
[task 2017-04-11T18:10:16.374611Z] 18:10:16    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at CountingMalloc, ICUReporter::Alloc, icu_58::PatternMap::add, icu_58::DateTimePatternGenerator::addPatternWithSkeleton
[task 2017-04-11T18:10:16.377550Z] 18:10:16    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at CountingMalloc, ICUReporter::Alloc, puti, setAvailableFormat

Comment 86

[Jorg K (CEST = GMT+2)]

Zibi or  Masatoshi-san, any idea what's going on here?

Some "close" missing? I have an udatpg_close() which was also in the old code.

Comment 87

[Jorg K (CEST = GMT+2)]

Attachment: 1351427-datetime.patch (v7c).

Refactoring lost the |udat_close(dateTimeFormat);|

Please use interdiff with (v7b) to check the one-line change.

Comment 88

[Jorg K (CEST = GMT+2)]

Comment on attachment 8857166 [details] [diff] [review]
1351427-datetime.patch (v7c).

Oh boy, I hastily added another udat_close() that was completely wrong, so back to the previous version and the previous question.

Comment 89

[Jorg K (CEST = GMT+2)]

Comment on attachment 8856631 [details] [diff] [review]
1351427-datetime.patch (v7b).

OK, this is the version that went for a spin on M-I and got backed out. So repeating the question from comment #86:

Zibi or  Masatoshi-san, any idea what's going on here?
Some "close" missing? I have an udatpg_close() which was also in the old code.

Comment 90

[Zibi Braniecki [:zbraniecki][:gandalf]]

Unfortunately, my C++ skills are still a bit too junior to reason about ICU related memory leaks.

:jfkthame or :emk may be able to put you on the right track.

Comment 91

[Masatoshi Kimura [:emk]]

How about using UniquePtr instead of managing the object lifetime manually?

Comment 92

[Jorg K (CEST = GMT+2)]

I don't know how to interpret your comment. What should be a UniquePtr? The log in comment #85 suggests that something allocated for/by udatpg_open wasn't cleaned up.

The bug is here
https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/intl/locale/OSPreferences.cpp#270
in GetDateTimeConnectorPattern(), it is missing the udatpg_close() call.

Comment 93

[Jorg K (CEST = GMT+2)]

Attachment: 1351427-datetime.patch (v7b) + fixing GetDateTimeConnectorPattern()

This should do it.

If you like it, you could push it to inbound together with bug 1355465.

Comment 94

[Jorg K (CEST = GMT+2)]

I've done another try run with the patch from bug 1355465 on top, just to be double sure ;-)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=15d2adb2e0bc0030286b41b9e3fe5dc85f90aca3

Comment 95

[Masatoshi Kimura [:emk]]

Comment on attachment 8857707 [details] [diff] [review]
1351427-datetime.patch (v7b) + fixing GetDateTimeConnectorPattern()

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

(In reply to Jorg K (GMT+2) from comment #92)
> I don't know how to interpret your comment. What should be a UniquePtr? The
> log in comment #85 suggests that something allocated for/by udatpg_open
> wasn't cleaned up.

I meant something like 
  UniquePtr<UDateTimePatternGenerator, decltype(&udatpg_close)> pg;
. You don't have to rewrite early return with this. But it's up to you

Comment 96

[Jorg K (CEST = GMT+2)]

Thanks for the review, I think this is fine.

Comment 97

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bbd4235a356
Use OSPreferences for C++ date/time formats. r=emk

Comment 98

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2bbd4235a356

Comment 99

[Eugene Savitsky]

Sorry for spamming, but when the change will be seen in Earlybird? I still see USA date format on Russian locale.

Comment 100

[Jorg K (CEST = GMT+2)]

You're in the wrong bug, for Thunderbird read bug 1344594 comment #18.

When using en-US software you will not see date/time format according to the Russian locale, you have to use a Russian localised version instead.

Also, Aurora has been dropped by Mozilla Firefox and Thunderbird. The existing "Earlybird 54" will not have this feature, details in the aforementioned comment.

Comment 102

[Liz Henry (:lizzard) (relman/hg->git project)]

Does this affect 54? Is it something you may want to uplift to 54 beta? I'm asking because this patch was mentioned in bug 1355307. Thanks!

Comment 103

[Zibi Braniecki [:zbraniecki][:gandalf]]

I doubt we can easily retrofit that as it bases on changes that I don't think are in 54.

Comment 104

[Jorg K (CEST = GMT+2)]

It may be possible, someone needs to try. I'm not building FF beta 54, so I can't tell for sure.

The change here mainly relies on OSPreferences.cpp/h which is already in place in 54. Compare:

Beta 54:
https://hg.mozilla.org/releases/mozilla-beta/log/tip/intl/locale/OSPreferences.cpp

Trunk 55:
https://hg.mozilla.org/mozilla-central/log/tip/intl/locale/OSPreferences.cpp

So I believe it has a good chance of working.

Let me give you some background. The C++-created date/time display only affects FF in two places: Certificate details and navigation history (attachment 8841316 [details]). Thunderbird is much more affected, since it uses those C++ dates in the Date/Received column which is always visible in a prime GUI location. So from a TB point of view it would be highly desirable to backport this since TB 54 since the en-US version of TB 54 is pretty much unusable for anyone wanting to change date/time formats. Of course localised versions are OK, so I don't know how many users are affected, I assume mostly international developers.

RyanVM has done some difficult backports in the past, so if you can win him for this project, I'm happy to assist him.