Introduce mozIntl.DateTimeFormat

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

(Blocks 1 bug, {dev-doc-needed})

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

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

Updated

2 years ago
Depends on: 1308329, 1329904
(Assignee)

Updated

2 years ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Depends on: 1339892
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1343768
(Assignee)

Comment 2

2 years ago
Initially, we intend to make this patch work on 3 platforms:

 - GTK (Linux)
 - Windows
 - Mac

Each of them has a bit different characteristics:

On GTK we ignore the input locale, since the only think we adapt is hourCycle, so we take the date/time pattern from ICU and alter it with the hourCycle from the OS *if* the pref has been manually set.

On Windows, we will take OS regional preferences if the app locale matches the regional preferences locale used in Windows.

At the moment the match is a bit weird, for example appLocale: ['pl-PL'], regional settings: ['pl'] matches properly, but ['en'] to ['en-US'] does not.
For that reason I think I'd like to use Language Negotiation (bug 1337694) to do our own matching and if we decide we match, just skip the locale argument when calling to Windows API.

On Mac, it seems that passing any locale to the Cocoa API returns patterns without current user prefs, even if the locale matches exactly.
So I believe we should do the same as on Windows and use our own language negotiation to see if the locale matches and if it does don't pass anything to the call for the pattern.

Now, the tricky part here is that we currently have APIs for retrieving the system locale on Mac and Windows, but both systems have separate selection of locale for regional settings.
It seems to me that we should identify what APIs return the regional preferences selected locale and match that against our app locale.

I'll look for that API on both, mac and windows, but maybe :jfkthame already knows what API would that be.

Either way, this bug depends on bug 1337694.
Depends on: 1337694
(Assignee)

Comment 3

2 years ago
:jfkthame, need a bit of your help. Now that we have langneg, I'd like to cast CFLocaleRef to nsACString and LWSTR to nsACString.
How do I cast those two?

Then I'll be able to compare the locale we use as "default" to the locale that we use in the app and decide if we want to take the regional settings or not.
Flags: needinfo?(jfkthame)
For CFLocaleRef, you should be able to get its canonical identifier using CFLocaleGetIdentifier; that returns a CFLocaleIdentifier, which is simply a typedef for CFStringRef. So then you can use CFStringGetCString to copy the string's contents into a char[] buffer, and wrap that with an nsDependentCString to use with string APIs -- or maybe just compare it using LowerCaseEqualsASCII.

One issue might be whether what CFLocale uses as an identifier actually matches what we use; e.g. I suspect for Chinese, it'll probably give you things like zh-Hans-CN and zh-Hant-TW, whereas I believe we use zh-CN and zh-TW. So a simple string comparison probably won't give us what we want.

For LWSTR in the Windows code, you should be able to cast an LWSTR to const char16_t*, as it's simply a string of 16-bit characters; then you can use NS_ConvertUTF16toUTF8 to get an 8-bit representation (or given what we know about the characters in locale identifiers, you can use the slightly cheaper NS_LossyConvertUTF16toASCII here). So if wstr is a Windows wide string (I guess you'll get it from ResolveLocaleName or something?), then

    NS_LossyConvertUTF16toASCII cstr(static_cast<const char16_t*>(wstr), wstrLen);

should give you an nsACString-compatible string to work with.
Flags: needinfo?(jfkthame)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 years ago
I think I got the right behavior on Mac.

On Windows, I don't understand what's going on, need to investigate more.
(Assignee)

Comment 7

2 years ago
I think I'm stuck. The code on Windows does the same thing as on Mac, but the result is vastly different.

In particular, on Windows, for some reason:

osPrefs.systemLocale; // en-US
osPrefs.getDateTimeFormat(1,1); "yyyy-mm-dd"
osPrefs.getDateTimeFormat(1,1,"en-US"); "mm/dd/yyyy"

both when I pass nullptr and LOCALE_NAME_USER_DEFAULT to GetLocaleInfoEx in case of a match.

also, in no configuration I can make it react to manual altering of the regional settings. I'm changing the clock from h24 to h12 and in no setup does the result changes. I'm confused.
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
Comment on attachment 8843134 [details]
Bug 1339650 - Introduce mozIntl.DateTimeFormat.

Victory!

This patch has it all, mac, windows and gtk!

The basic assumption I made is:
 - you either pass a locale or we'll take it from the app
 - then we match that locale against system locale
 - if it's a match we'll return what the bindings want to get pattern with regional settings preferences
 - otherwise we'll get the prefs for a locale

I think it's ready for feedback round :)

Two things still not there:
 - If there's no match, maybe we should just return false and use our ICU?
 - the locale we use to get the connector pattern on Windows should use negotiated locale, not the input locale, but my attempt to cast LPWSTR to char* failed.
Attachment #8843134 - Flags: feedback?(jfkthame)
(Assignee)

Updated

2 years ago
Blocks: 1346877
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 years ago
I cleaned up the patch a bit and I think it's ready for review.

Comment 12

2 years ago
mozreview-review
Comment on attachment 8843134 [details]
Bug 1339650 - Introduce mozIntl.DateTimeFormat.

https://reviewboard.mozilla.org/r/116906/#review125010

A couple of issues with this, so clearing r? until it's fixed up.

::: intl/locale/LocaleService.cpp:369
(Diff revision 4)
> +bool
> +LocaleService::LanguagesMatch(const nsCString& aRequested,
> +		              const nsCString& aAvailable)
> +{
> +  nsTArray<nsCString> requestedLocales;
> +  requestedLocales.AppendElement(aRequested);
> +  nsTArray<nsCString> availableLocales;
> +  availableLocales.AppendElement(aAvailable);
> +  nsTArray<nsCString> supportedLocales;
> +
> +  FilterMatches(requestedLocales, availableLocales, LangNegStrategy::Lookup, supportedLocales);
> +  return !supportedLocales.IsEmpty();
> +}

This seems an expensive approach to testing whether the language subtags of the two codes match (which is what it ultimately achieves, right?)

How about adding a helper to the Locale class:

    bool LanguagesMatch(const Locale& aOther)
    {
      return mLanguage.Equals(aOther.mLanguage,
                              nsCaseInsensitiveCStringComparator());
    }

and then this becomes simply:

    return Locale(aRequested).LanguagesMatch(Locale(aAvailable));

or something like that.

::: intl/locale/mac/OSPreferences_mac.cpp:71
(Diff revision 4)
> +  nsCString reqLocale;
> +  nsCString systemLocale;

Use nsAutoCString for these to avoid heap allocations.

::: intl/locale/mac/OSPreferences_mac.cpp:82
(Diff revision 4)
> -    return CFLocaleCopyCurrent();
> +    LocaleService::GetInstance()->GetAppLocale(reqLocale);
> +  } else {
> +    reqLocale.Assign(aLocale);
>    }
> +
> +  bool match = LocaleService::GetInstance()->LanguagesMatch(reqLocale, systemLocale);

Hmm, AFAICS there's really no reason for the `LanguagesMatch` helper to be an instance method at all.

I'd suggest declaring it `static` in the class, and then you'll simply call it as

    bool match = LocaleService::LanguagesMatch(...);

::: intl/locale/windows/OSPreferences_win.cpp:76
(Diff revision 4)
> +  nsCString reqLocale;
> +  nsCString systemLocale;

nsAutoCString

::: intl/locale/windows/OSPreferences_win.cpp:86
(Diff revision 4)
> +    LocaleService::GetInstance()->GetAppLocale(reqLocale);
> +  } else {
> +    reqLocale.Assign(aLocale);
> +  }
> +
> +  bool match = LocaleService::GetInstance()->LanguagesMatch(reqLocale, systemLocale);

bool match = LocaleService::LanguagesMatch(...)

(assuming LanguagesMatch is made `static` as suggested above)

::: intl/locale/windows/OSPreferences_win.cpp:92
(Diff revision 4)
> +  nsAutoString localeNameBuffer;
> +  localeNameBuffer.AppendASCII(reqLocale.BeginReading(), reqLocale.Length());
> +  LPWSTR localeName = (LPWSTR)localeNameBuffer.BeginReading();
> +  return localeName;

This isn't safe: `localeNameBuffer` will be destroyed when it goes out of scope, at which point the `localeName` pointer being returned will be pointing at freed memory.

Rather than a `Create...` helper here (which implies allocating and returning an object of some kind, which the caller would then need to free -- but that doesn't apply in the DEFAULT case), I think you should just declare a sufficiently-large WCHAR[] buffer in the caller and pass to this function. Then this function (renamed `GetWindowsLocaleFor`) will either return the `LOCALE_NAME_USER_DEFAULT` value or it will copy the `reqLocale` string into the caller-supplied buffer, and return a pointer to it.
Attachment #8843134 - Flags: review?(jfkthame)
(Assignee)

Comment 13

2 years ago
Thank you!

I'll work on applying your feedback.

:jfkthame, can you also help me with the LPWSTR to nsACString conversion for the GetDateTimeConnectorPattern on Windows?
Flags: needinfo?(jfkthame)

Comment 14

2 years ago
Hmm, LPWSTR is a wide-string, just like the the internal string buffer of nsAutoString. Usually we use: NS_ConvertUTF16toUTF8() if you want UTF-8, or otherwise: NS_LossyConvertUTF16toASCII(). I'd have to see the exact snippet to be more helpful.
Comment hidden (mozreview-request)
(Assignee)

Comment 16

2 years ago
Updated the patch to your feedback.

Thank you :Jorg K for help!

Re-requesting review and triggered try build.
Flags: needinfo?(jfkthame)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8843134 [details]
Bug 1339650 - Introduce mozIntl.DateTimeFormat.

https://reviewboard.mozilla.org/r/116906/#review125142

::: intl/locale/LocaleService.h:151
(Diff revisions 4 - 5)
>                            const nsACString& aDefaultLocale,
>                            LangNegStrategy aLangNegStrategy,
>                            nsTArray<nsCString>& aRetVal);
>  
> -  bool LanguagesMatch(const nsCString& aRequested,
> +  static bool LanguagesMatch(const nsCString& aRequested,
> -		      const nsCString& aAvailable);
> +		   		      const nsCString& aAvailable);

oops, stray tabs?

::: intl/locale/LocaleService.cpp:373
(Diff revisions 4 - 5)
>  
>  bool
>  LocaleService::LanguagesMatch(const nsCString& aRequested,
>  		              const nsCString& aAvailable)
>  {
> -  nsTArray<nsCString> requestedLocales;
> +  return Locale(aRequested, true).Matches(Locale(aAvailable, true), true);

This is a bit awkward... it's not obvious what those three `true` params are doing. It would be slightly better as

    return Locale(aRequested, true).MatchesLanguage(Locale(aAvailable, true));

where at least `true` only has one meaning, and the other option needed is implemented as a separate method name.

::: intl/locale/LocaleService.cpp:583
(Diff revisions 4 - 5)
>    auto subtagMatches = [](const nsCString& aSubtag1,
>                            const nsCString& aSubtag2) {
>      return aSubtag1.EqualsLiteral("*") ||
>             aSubtag2.EqualsLiteral("*") ||
>             aSubtag1.Equals(aSubtag2, nsCaseInsensitiveCStringComparator());
>    };

If you take this out of the `Matches()` method and make it a static helper function

    static bool
    SubtagMatches(const nsCString& aSubtag1, ...)

at file scope, then you can also use it in a new `MatchesLanguage()` method without having to repeat it, and avoid adding the `aLoose` param to this one.

::: intl/locale/windows/OSPreferences_win.cpp:92
(Diff revisions 4 - 5)
>    nsAutoString localeNameBuffer;
>    localeNameBuffer.AppendASCII(reqLocale.BeginReading(), reqLocale.Length());
> -  LPWSTR localeName = (LPWSTR)localeNameBuffer.BeginReading();
> +  aRetVal= (LPWSTR)localeNameBuffer.BeginReading();

No, this won't do what you want; it puts the name into a local buffer (the autostring) which is about to disappear, and then updates the aRetVal pointer (not the buffer it points at), which is purely local to the method. So the caller will never see the result at all!

What you want is to check that the length of `reqLocale` isn't going to overflow, and then use something like

    UTF8ToUnicodeBuffer(reqLocale, (char16_t*)aRetVal);

from nsReadableUtils.h to copy the text to the caller's buffer (which aRetVal points to).

::: intl/locale/windows/OSPreferences_win.cpp:135
(Diff revisions 4 - 5)
>    // Set 'str' to point to the string we will use to retrieve patterns
>    // from Windows.
>    nsAutoString tmpStr;
>    nsAString* str;
>    if (isDate && isTime) {
> -    //XXX: We should get it for localeName
> +    if (!GetDateTimeConnectorPattern(NS_ConvertUTF16toUTF8(localeName), aRetVal)) {

Does this build as-is? I'd have thought it might need a `(const char16_t*)` cast applied to `localeName`.
Attachment #8843134 - Flags: review?(jfkthame)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

2 years ago
mozreview-review-reply
Comment on attachment 8843134 [details]
Bug 1339650 - Introduce mozIntl.DateTimeFormat.

https://reviewboard.mozilla.org/r/116906/#review125142

> oops, stray tabs?

nope, aligned to the convention of the file.

> No, this won't do what you want; it puts the name into a local buffer (the autostring) which is about to disappear, and then updates the aRetVal pointer (not the buffer it points at), which is purely local to the method. So the caller will never see the result at all!
> 
> What you want is to check that the length of `reqLocale` isn't going to overflow, and then use something like
> 
>     UTF8ToUnicodeBuffer(reqLocale, (char16_t*)aRetVal);
> 
> from nsReadableUtils.h to copy the text to the caller's buffer (which aRetVal points to).

Ugh, strings are tricky. Cross-platform strings are trickier :)

Hope this will work.

> Does this build as-is? I'd have thought it might need a `(const char16_t*)` cast applied to `localeName`.

Yeah, it does! try build is green! :)
Comment hidden (mozreview-request)

Comment 22

2 years ago
mozreview-review
Comment on attachment 8843134 [details]
Bug 1339650 - Introduce mozIntl.DateTimeFormat.

https://reviewboard.mozilla.org/r/116906/#review125484

::: intl/locale/LocaleService.cpp:579
(Diff revisions 5 - 8)
>      if (mVariant.IsEmpty()) {
>        mVariant.Assign(NS_LITERAL_CSTRING("*"));
>      }
>    }
>  }
> -
> +static bool

add a blank line before this, please

::: intl/locale/LocaleService.cpp:599
(Diff revisions 5 - 8)
> +}
> +
> +bool
> +LocaleService::Locale::LanguageMatches(const LocaleService::Locale& aLocale) const
> +{
>  

and drop the spare blank line here

::: intl/locale/windows/OSPreferences_win.cpp:75
(Diff revisions 5 - 8)
>  void
>  GetWindowsLocaleFor(const nsACString& aLocale, LPWSTR aRetVal)

Make this return an LPWSTR pointer, as we may want to return the special `LOCALE_NAME_USER_DEFAULT` or we may want to point to the caller-supplied buffer. So,

    LPWSTR
    GetWindowsLocaleFor(const nsACString& aLocale, LPWSTR aBuffer)
    {
      ...
    }

::: intl/locale/windows/OSPreferences_win.cpp:94
(Diff revisions 5 - 8)
>    if (match) {
>      aRetVal = LOCALE_NAME_USER_DEFAULT;
> +    return;
>    }
>  
> -  nsAutoString localeNameBuffer;
> +  const uint32_t len = CalcUTF8ToUnicodeLength(reqLocale);

Ugh... I suppose this is technically correct, but it's an extra operation that we really don't need. Keeping in mind that the number of UTF16 code units for a string can never be larger than the number of UTF8 code units, we can just say

    if (reqLocale.Length() >= 100) {
      return LOCALE_NAME_USER_DEFAULT;
    }

and it'll be safe. In theory the UTF16 form could be *shorter* and might fit even when the UTF8 length is longer than this, but given that locale tags are pure ASCII, that won't matter for any valid tag.

(Oh, and instead of an arbitrary `100`, here and in the code that calls it, let's use the `LOCALE_NAME_MAX_LENGTH` constant from Windows, as that seems an appropriate thing.)

::: intl/locale/windows/OSPreferences_win.cpp:96
(Diff revisions 5 - 8)
> -  aRetVal= (LPWSTR)localeNameBuffer.BeginReading();
> +    aRetVal = LOCALE_NAME_USER_DEFAULT;
> +    return;

Setting `aRetVal` like this doesn't work, it's not being returned to the caller. Return a pointer as the function result:

    return LOCALE_NAME_USER_DEFAULT;

::: intl/locale/windows/OSPreferences_win.cpp:100
(Diff revisions 5 - 8)
> +  UTF8ToUnicodeBuffer(reqLocale, (char16_t*)aRetVal);
>  }

And here, return a pointer to the buffer that we just copied the locale string into:

    return aBuffer;

(assuming the input param name is changed from `aRetVal` to `aBuffer` as suggested above).
Attachment #8843134 - Flags: review?(jfkthame)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

2 years ago
Ugh, the latest version fails on Windows because of:

```
WCHAR localeName[LOCALE_NAME_MAX_LENGTH];
localeName = GetWindowsLocaleFor(aLocale, localeName);
```

I've never worked with a pattern where I pass a buffer, and accept a return value and either can be the value assigned to localeName variable. I'm not sure what I'm doing wrong.
Flags: needinfo?(jfkthame)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #24)
> Ugh, the latest version fails on Windows because of:
> 
> ```
> WCHAR localeName[LOCALE_NAME_MAX_LENGTH];
> localeName = GetWindowsLocaleFor(aLocale, localeName);
> ```
> 
> I've never worked with a pattern where I pass a buffer, and accept a return
> value and either can be the value assigned to localeName variable. I'm not
> sure what I'm doing wrong.

You want to assign the return value of GetWindowsLocaleFor() to a separate LPWSTR variable, something like:

  WCHAR buffer[LOCALE_NAME_MAX_LENGTH];
  LPWSTR localeName = GetWindowsLocaleFor(aLocale, buffer);

(untested, but I think it should be about right...)
Flags: needinfo?(jfkthame)
Comment hidden (mozreview-request)
(Assignee)

Comment 27

2 years ago
Oh, ok! yes. This work on my locale machine, thanks :)

Re-requesting r?
(Assignee)

Updated

2 years ago
Blocks: 1351427
(Assignee)

Comment 28

2 years ago
:jfkthame, do you have any ETA for this one? I'd like to start transitioning Firefox UI to use it soon.
Flags: needinfo?(jfkthame)

Comment 29

2 years ago
mozreview-review
Comment on attachment 8843134 [details]
Bug 1339650 - Introduce mozIntl.DateTimeFormat.

https://reviewboard.mozilla.org/r/116906/#review127632

LGTM, let's do it. :)

::: toolkit/components/mozintl/mozIMozIntlHelper.idl:29
(Diff revision 10)
>     * Adds a PluralRules constructor to the given object.  This function may only
>     * be called once within a realm/global object: calling it multiple times will
>     * throw.
>     */
>    [implicit_jscontext] void addPluralRulesConstructor(in jsval intlObject);
> +  [implicit_jscontext] void addDateTimeFormatConstructor(in jsval intlObject);

Looks like this needs its own comment, or the comment above PluralRules should be made more general to cover both methods.
Attachment #8843134 - Flags: review?(jfkthame) → review+
Comment hidden (mozreview-request)

Comment 31

2 years ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/55647bd1ea9d
Introduce mozIntl.DateTimeFormat. r=jfkthame

Comment 32

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/55647bd1ea9d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

2 years ago
Flags: needinfo?(jfkthame)

Comment 33

2 years ago
Zibi, one question please. I've read you post on dev-platform, quote:
"mozIntl.DateTimeFormat will respect that and show the time in the current UI locale, but with this adjustment."

Where in a current Nightly can I observe this at work? Or do bug 1354442 and bug 1354445 have to be fixed first?

Updated

2 years ago
Blocks: 1355465

Updated

2 years ago
No longer blocks: 1351427, 1355465
(Assignee)

Comment 34

2 years ago
Is there a chance to get some help with documenting mozIntl? In particular, the mozIntl.createDateTimeFormat would be important to document as the preferred way to interantionalize date/time in Firefox UI. The dateStyle/timeStyle is currently proprietary, although we do have a proposal for it in ECMA402 - https://github.com/zbraniecki/proposal-ecma402-datetime-style
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.