Closed Bug 1308329 Opened 8 years ago Closed 7 years ago

Extend OSPreferences API to cover regional settings

Categories

(Core :: Internationalization, defect, P3)

Unspecified
Other
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed)

Attachments

(9 files, 7 obsolete files)

18.17 KB, image/png
Details
24.24 KB, image/png
Details
209.12 KB, image/png
Details
208.41 KB, image/png
Details
26.19 KB, image/png
Details
20.71 KB, image/png
Details
181.27 KB, image/png
Details
94.65 KB, image/png
Details
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
This is a spin-off from bug 1301640 needed, among others, for bug 1303579.

There's a set of preferences in the OS that should inform our Intl formattings.
Things like:

 - hour cycle format
 - calendar
 - first day of a week
 - time zone
 - region
 - temperature format
 - numbering system
 - number separator (grouping/decimal)
 - currency
 - measurement units (Metric, US, UK)
 - date formats short/medium/long/full
 - time formats short/medium/long/full
 - dayperiod names
 - time display with/without seconds
 - flash the time separators
 - show dayPeriod
 - show the day of the week
 - show date

I gathered this list from Mac OS preferences. Windows, Linux, Android and Window Mobile have some subset of those preferences.

We want to add a C++ API that will retrieve some of those settings from the OS and allow us to build public facing API's on top of that.

In the end, we will expose some of those items to our chrome APIs, and standardize even smaller subset to be exposed to web content (most in form of BCP47 extension keys).

The above list is exhaustive, but worth keeping in mind as we design this API.

The way we will expose it in user facing APIs will be different depending on the exact API. For example, a subset of them will be available only via extension keys on `navigator.locales`:
 - hour cycle
 - timezone
 - region
 - numbering system
 - currency

other elements, like date and time format patterns, may not be exposed to web content, but may be made available for chrome content.

The way I envision this API is similar to nsIDateTimeFormat, maybe nsIIntlOSPreferences with methods such as:

 - gethourCycle
 - getTimezone
 - getRegion
 - getNumberingSystem
 - getCurrency
 - getDateFormat

each method may return one of the valid bcp47 values for the setting if one exists, or a string where it makes sense.
The idea is that it will *only* return a value if the manual override of the setting has been defined by the user and "null" value when the user is not overriding per-locale default.

This will allow us to follow locale defaults in all cases where the user did not specify otherwise.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #0)
> The idea is that it will *only* return a value if the manual override of the
> setting has been defined by the user and "null" value when the user is not
> overriding per-locale default.

Can we actually tell whether the user has modified these settings? From a (brief, admittedly) look at the OS X docs, I can see how we can retrieve the date-formatting patterns, for example, but I don't see a way to determine whether the pattern we get is the OS's default for the locale, or something the user has tweaked.

What should we do when the OS defaults differ from ICU/CLDR defaults -- do we retrieve the OS defaults and explicitly pass them to the ICU functions (as if they were customizations) so as to match the platform behavior by default?
Flags: needinfo?(gandalf)
So, testing the UI in Mac OS it seems that they reset overrides when you switch region.

This means, that I think we can get a pretty good foundation if we go for:

0) If in non-chrome code and fingerprinting is off - return null
1) Read the OS preference
2) Compare it to the default for the given locale from ICU/CLDR
3) If it differs, return it
4) If it doesn't, return null.

Does it sound reasonable?
Flags: needinfo?(gandalf) → needinfo?(jfkthame)
I started poking into Gnome.

Seems like the value can be read from `gsettings get org.gnome.desktop.interface clock-format` and can take values '12h' or '24h'.
It seems that changing region doesn't affect it, and resetting just sets it to '24h'.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #3)
> I started poking into Gnome.
> 
> Seems like the value can be read from `gsettings get
> org.gnome.desktop.interface clock-format` and can take values '12h' or '24h'.
> It seems that changing region doesn't affect it, and resetting just sets it
> to '24h'.

Yeah, at the C++ level it looks like it's simple to read it through the gsettings API. Though for users who aren't running a Gnome desktop, I wonder if that still holds true? Other desktop environments may have their own separate settings.

On Mac OS, it'll be a bit more of a pain, as I think the only way to determine the 12/24hr setting is to use something like [NSDateFormatter dateFormatFromTemplate:options:locale:] and then examine the resulting format string.

Windows has a locale info constant LOCALE_ITIME that I think we can use with GetLocaleInfoEx to get the 12/24hr setting.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)
> 0) If in non-chrome code and fingerprinting is off - return null
> 1) Read the OS preference
> 2) Compare it to the default for the given locale from ICU/CLDR
> 3) If it differs, return it
> 4) If it doesn't, return null.

Why would we bother with comparing to the ICU/CLDR default? We could just return the OS-pref value in all cases (except in anti-fingerprinting mode). If it's the same as ICU's default, using it won't make any difference to the result anyway.
Flags: needinfo?(jfkthame)
I'll look more into the three OS-es and see if I can find a way to distinguish between not-set and set-to-default.

I know that that's what we did in FxOS, and I think I saw something similar in Android or MacOS.

(In reply to Jonathan Kew (:jfkthame) from comment #4)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)
> > 0) If in non-chrome code and fingerprinting is off - return null
> > 1) Read the OS preference
> > 2) Compare it to the default for the given locale from ICU/CLDR
> > 3) If it differs, return it
> > 4) If it doesn't, return null.
> 
> Why would we bother with comparing to the ICU/CLDR default? We could just
> return the OS-pref value in all cases (except in anti-fingerprinting mode).
> If it's the same as ICU's default, using it won't make any difference to the
> result anyway.

The reason to do that is two-fold for me:

1) I believe that it will be possible to distinguish, at least in some cases, "non set" from "set to a value matching the default" setting.
2) We should try to minimize the amount of bits set from the OS in scenarios where bits are not set.

The main reason is that we want to use a language fallback chain and each language may have it's own preferences.

Taking h12/h24 as an example, and a fallback chain ['pl', 'en-US'] - if I did not set 'h12/h24' manually, and a given app does not have 'pl', it should fallback to 'en-US' including its intl settings (date time format, hourCycle etc.).

If we always return all possible values for the first language, then our fallback chain will look like this:

['pl-hc-h24', 'en-US-hc-h24'] - which is what I want it to look like *only* if the user explicitly defined hourCyle in the OS preferences.

If he didn't, I'd love it to be:

['pl', 'en-US'] and then Intl APIs will negotiate the right locale and its settings.

The other is, once again, fingerprinting. Even if we don't turn off the feature with a pref, it would be good to minimize the amount of bits we expose about the user.
And since 95% of users will never change their defaults, it would be good to return nothing for 95% of users (who use their primarily language and hourCycle settings matching its default).

Does it sound like a good option?
Flags: needinfo?(jfkthame)
It makes sense, but in most cases I'm doubtful we will be able to distinguish defaults from user-set values that happen to match the default. (If I'm wrong about that, great!)

(Just thinking aloud, here....) I'm not even sure it's a distinction that would make sense to users, actually. Suppose I open the Language & Region settings panel (I'm on OS X), and I see that there's a checkbox labelled "Time Format: [ ] 24-hour time". I click it, mostly out of curiosity, and notice that the example times displayed below change, and so does the clock in my menu bar. But I don't really want to switch to 24-hr times, so I click again to clear the checkbox.

OK, what's the state of the preference? Have I now explicitly chosen to have 12-hr times everywhere, even if locale fallback finds a locale where 24-hr is the default? Or have I simply cancelled my experimental click on the checkbox, reverting to default behavior?

- - - - -

Another complication I just noticed, by the way. We may not be able to have a simple IntlOSPrefs.getHourCycle method, at least on Windows. On MacOS, there's a single checkbox in the Language & Region panel that toggles between 12h/24h, but on Win10, I don't see that; instead, as a user I can choose "long" and "short" time formats independently, and there's nothing to prevent me choosing 12h display for one of them, and 24h for the other. So what should getHourCycle() return? I guess we could pass a 'shortTime' / 'longTime' parameter to it, to specify which of the formats to check... reasonable?
Flags: needinfo?(jfkthame)
> It makes sense, but in most cases I'm doubtful we will be able to distinguish defaults from user-set values that happen to match the default. (If I'm wrong about that, great!)

Let's see what I can get.

> OK, what's the state of the preference?

If your OS doesn't allow you to set "follow the defaults" option here, then yes, you manually set an option.

For that reason the only available options are:

 - if your OS resets the value after language change (MacOS does), we can assume that by unchecking you're just returning to the default for your locale - after all, if you really want "12h clock" and you'll ever switch to 24h clock locale, you'll have to switch this option manually again, in which case, we'll know that you're in "de" language and "12h clock" so it's your preference, and not the default.

 - if your OS doesn't reset (Gnome), we can only guess and we can just do our best job at minimizng the amount of bits we share - if you're in 24h clock locale, and you have 24h clock, we don't have to share the bit. If you're in 12h clock, we have to.

> and there's nothing to prevent me choosing 12h display for one of them, and 24h for the other. So what should getHourCycle() return? I guess we could pass a 'shortTime' / 'longTime' parameter to it, to specify which of the formats to check... reasonable?

Interesting question. My idea was to follow that (short/long date/time) because MacOS also has it, and have a combo for each in this API eventually as well and then make our code rather follow that than construct date/time individually.

That would also solve the "show seconds in time" option.

But in the context of getHourCycle - the setting for that in Win10 is a really bad UX and much worse than MacOS, Android, Gnome or Win7 IIRC. Exposing pattern skeletons to users looks really bad. I expect that they'll change that in the upcoming revisions.

We can either:
1) Follow one of them (say, short time)
2) Return a value only if both are the same (short and long)
3) Ignore the setting on Win10 because it seems like it's not something users are expected to mingle with

By that I mean, I guess, that we can pick the best strategy per-OS.

And eventually, once we have sth like:

nsOSIntlPreferences.getShortTimeFormat();
nsOSIntlPreferences.getLongTimeFormat();

those will follow the OS more closely.
I've been looking at this a bit, and as a first step, here's a possible approach to a low-level C++ API to access the platform-specific settings (only partially implemented as yet). The idea is that we can then build whatever higher-level, script-accessible API we want on top of this in a platform-neutral way. Does this seem like a useful place to start?
Attachment #8801109 - Flags: feedback?(gandalf)
Part 2, then, is to implement nsIIntlOSPreferences to expose these settings to chrome JS. Not sure if this is the most useful way to expose them, though... e.g. for date & time formats, this currently returns "pattern" strings, but the ECMAScript Intl API uses with a very different model of 'options' to control date/time formatting.
Attachment #8801110 - Flags: feedback?(gandalf)
With the two patches above, it should be possible to do things like

  var intlOSPrefs = Cc["@mozilla.org/intl/ospreferences;1"].getService(Ci.nsIIntlOSPreferences);
  intlOSPrefs.HourCycle()
  --> returns 12 or 24, according to the preferred hourCycle of the default locale

  var isDefault = {};
  intlOSPrefs.HourCycle(isDefault);
  --> returns 12 or 24, and sets isDefault.value to true or false

  intlOSPrefs.HourCycle({}, "de-CH");
  --> returns value for the given locale instead of the system default locale

  intlOSPrefs.DateFormat(intlOSPrefs.dateFormatExtended);
  --> returns a pattern such as "EEEE, d MMMM y" (depending on the locale, etc)

and so on.

(Note that the methods will throw an error if they cannot look up an OS setting -- e.g. because the platform doesn't provide one -- so callers must handle this and fall back to a suitable default. Perhaps that's not the most useful API, though; maybe these methods should always return a valid answer, together with an indication of whether it's the default or a custom setting. Yes, I think that would be more usable...)
That's awesome! Thanks so much for kicking it off Jonathan!

I'd like to get a couple days to think about the interactions between this and Intl API, in particular with regards to date/time formatting.

The hourCycle is easy: we take what we get here, use `Intl.Locale` API to build `navigator.locales` and pass it into Intl APIs after we land the spec/impl change to react to hc extension key.

But for date/time formatting, we need to decide how we want users to use it, and how we envision it for web world. Currently ICU just allows you to build any date/time string and there's no way to pass it a pattern that he user chose.

Maybe, we could use `Date.prototype.toLocaleTimeString`, `Date.prototype.toLocaleString` and `Date.prototype.toLocaleDateString` - they correspond quite well to date, time and datetime.
On the other hand, they don't have short/long style.

Also, that would require us to make a change to the spec to make those behave similarly to how timeZone works - if not passed explicitly, try to get it from the host environment - not sure if ECMA402 will be excited about getting this result to be non-deterministic.

On the other hand if we don't do this, we may want to add some way to retrieve the OS date/time preferences in form of options for Intl API.

For example the API like this:

```
intlOSPrefs.DateFormat(intlOSPrefs.dateFormatExtended);
  --> returns {weekday: 'long', month: 'long', day: 'numeric', year: 'numeric'}
```

Gnome doesn't seem to allow for any settings here, Windows seems to only expose short/long and MacOS short/medium/long/full so we'll have to do some guess work around it.
Flags: needinfo?(gandalf)
Flags: needinfo?(gandalf)
Flags: needinfo?(gandalf)
Flags: needinfo?(gandalf)
Actually, Win10 appears to support full customization of date/time formats (see attached image); there are similar panels for numbers and currency, too.
Priority: -- → P3
See Also: → 1324133
Does anyone know if there's update about this issue?
See Also: → 625369
See Also: 625369
See Also: → 1325751
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5)
> The reason to do that is two-fold for me:
> 
> 1) I believe that it will be possible to distinguish, at least in some
> cases, "non set" from "set to a value matching the default" setting.
> 2) We should try to minimize the amount of bits set from the OS in scenarios
> where bits are not set.
> 
> The main reason is that we want to use a language fallback chain and each
> language may have it's own preferences.

I really doubt that is something preferable. Why on earth would someone want to change their currency or time format to something else just because the website isn't set set up do display every string localized into their language. There's no logic in that - you have the currency you have, and you think in the date format you always think in.

> 
> Taking h12/h24 as an example, and a fallback chain ['pl', 'en-US'] - if I
> did not set 'h12/h24' manually, and a given app does not have 'pl', it
> should fallback to 'en-US' including its intl settings (date time format,
> hourCycle etc.).
> 
> If we always return all possible values for the first language, then our
> fallback chain will look like this:
> 
> ['pl-hc-h24', 'en-US-hc-h24'] - which is what I want it to look like *only*
> if the user explicitly defined hourCyle in the OS preferences.
> If he didn't, I'd love it to be:
> 
> ['pl', 'en-US'] and then Intl APIs will negotiate the right locale and its
> settings.

I'd expect if only a small minority get anything but the shorter ('en-US' type) values, it would appear likely the customizes would end up broken in the real world, having to settle for some default as developers didn't consider them.

> 
> The other is, once again, fingerprinting. Even if we don't turn off the
> feature with a pref, it would be good to minimize the amount of bits we
> expose about the user.
> And since 95% of users will never change their defaults, it would be good to
> return nothing for 95% of users (who use their primarily language and
> hourCycle settings matching its default).

Well that makes the fingerprinting issue *worse*, not better. The poor bastards having the changed defaults are very identifiable then.
(In reply to Magnus Melin from comment #21)

> I really doubt that is something preferable. Why on earth would someone want
> to change their currency or time format to something else just because the
> website isn't set set up do display every string localized into their
> language. There's no logic in that - you have the currency you have, and you
> think in the date format you always think in.

I don't see a connection between what your wrote and the quote from me that you pasted above it.

> I'd expect if only a small minority get anything but the shorter ('en-US'
> type) values, it would appear likely the customizes would end up broken in
> the real world, having to settle for some default as developers didn't
> consider them.

a) Vast majority of users do not modify any preferences.
b) The way we designed the Intl APIs is that lack of bit set does not break anything. If you don't set 'hc-h24' bit we'll take the default, not break.

And the whole idea behind Intl APIs is that the developer doesn't have to consider any particular case, or predict all combination, but rather he accepts that the internationalized output may be anything.
 
> Well that makes the fingerprinting issue *worse*, not better. The poor
> bastards having the changed defaults are very identifiable then.

The choice we're working on here is how to expose user selected preferences. If we do, it's fingerprintable. If we don't, users complain about the web not following their preferences.

The question is if it's better to make every user identifiable (by creating an API that always takes OS settings), never (by not listening to OS preferences at all like we do for the Web right now), or only when they modify (like I'm proposing).

Arguing that exposing everyone's setting will somehow reduce the fingerprintability is not in line with logic.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #22)
> I don't see a connection between what your wrote and the quote from me that
> you pasted above it.

You wanted a fallback chain for each language to have it's own formatting prefs (like h12/h24), but I'm just saying that's not how people work. Just because I want the app in English I do not want the regional settings for it. 

> Arguing that exposing everyone's setting will somehow reduce the
> fingerprintability is not in line with logic.

Like you say most people don't change defaults, so these would only be defaults you're sending out instead. Sending out defaults isn't adding anything to fingerprinting, but if you remove them the ones that have changed defaults are more easily identifiable as there's no noise. So when returning null for defaults you're giving out one more data point than without it: this user is using defaults. If you send out something, a fingerprinter have to use heuristics to figure out if that's a default or not.
> You wanted a fallback chain for each language to have it's own formatting prefs (like h12/h24), but I'm just saying that's not how people work. Just because I want the app in English I do not want the regional settings for it. 

Not exactly. Read more about the proposal for navigator.locales (and Intl.Locale) here:
 - https://github.com/tc39/ecma402/issues/68
 - https://github.com/tc39/ecma402/issues/105
 - https://github.com/tc39/ecma402/issues/106

> Sending out defaults isn't adding anything to fingerprinting, but if you remove them the ones that have changed defaults are more easily identifiable as there's no noise.

That's not how it works. If you have 10 users and we send hour12/24 pref for each one, we don't add noise. We add fingerprinting. Now, if we remove that bit for 9 users, and only leave it for the one that changed h12 to h24, we didn't make him stick out more. We just gave one bit of information on one user. Adding bits for the other 9 doesn't reduce anything.

As I mentioned in the other thread, this is really not the place to discuss the approach. If you disagree with it, please, comment in the ecma402 threads. That's where we will eventually try to resolve it for the ECMA402 standard :)
Blocks: 1301640
Ok, since this is not blocking bug 1301640, and we know better that both ECMA402 and ICU are interested in this, but it'll take them time to move forward, I suggest we implement our own chrome-only solution that will work both in C++ and JS.

I propose the following steps:

1) Implement mozIntl.DateTimeFormat which is a wrapper on top of Intl.DateTimeFormat
2) Add style: long|medium|short|narrow, date-long|date-medium|date-short|date-narrow, time-long|time-medium|time-short|time-narrow to mozIntl.DateTimeFormat
3) Create some sane default `options` objects for all 12 styles
4) Allow localizers to override any of those defaults for their locale
5) Implement intlOSPrefs feature that attempts to take OS settings for options for those 12 styles

:jfktheme - does it sound like a solution?

Open questions:

a) If intlOSPrefs will *always* return all values because it cannot distinguish between OS defaults and user selected ones, that means that we will never follow CLDR defaults, and we will only follow localizer provided data in cases where intlOSPRefs failed to extract info. Is that ok?

My vision was to get in order:
1) developer specified
2) user specified in the OS
3) localizer provided
4) hardcoded sane defaults (for style: ...)
5) CLDR defaults

But that only makes sense if each step provides limited information (for example, developer specifies month to be long, but doesn't say anything about other options).
If step (2) will return all the bits no matter what, then we're basically reduced to:

1) developer specified
2) OS provided

Is that ok for now?

:pike, :mkato, :stas - thoughts?
Flags: needinfo?(stas)
Flags: needinfo?(m_kato)
Flags: needinfo?(l10n)
s/since this is not blocking/since this is no*w* blocking/
No longer blocks: 1301640
I'd like to know coverage for both OSes and CLDR for the locales we have on desktop and android, and also what happens in cases where we don't have coverage.

The intent of that being that we need to ensure that our algorithm provides some sane values for all our locales.
Flags: needinfo?(l10n)
I think that it is good for this step.  We need some test cases for this.
Flags: needinfo?(m_kato)
For the record:

Please note that after landing of bug 1301640, the date and time format on Windows is determined exclusively by the Windows "format locale" which you can set under "Control Panel > Region and Language, Formats". Individual modifications applied to the "format locale" are ignored. Note that Windows also has a "system locale" under "Control Panel > Region and Language, Administrative".

localeService->GetApplicationLocale() returns the Windows "format locale". It doesn't look at what language pack is installed or what general.useragent.locale contains. Therefore dates/times displayed via ICU/CLDR using the C++ interface follow the Windows "format locale".

As least on Windows, bug 1301640 comment #117:
  the browser locale taken from `localeService->GetApplicationLocale` which
  should be the same as `general.useragent.locale`
is not correct.

Detailed experiments are described in 1301640 comment #144.

Note that in Firefox (to my knowledge) the only dates/times displayed using the C++ interface and ICU/CLDR can be found when displaying validity dates/times in the details of a certificate.
No longer blocks: 1332207
I spun off a smaller bug out of this and mozLocaleService.

The new bug 1333184 will just introduce OSPreferences API for retrieving system locales. This bug will be a bigger follow up to handle retrieving regional settings.

I prefer to split it because bug 1333184 blocks mozLocaleService while this bug blocks mozIntl.DateTimeFormat.
Summary: Create an API that retrieves Intl related OS preferences → Extend OSPreferences API to cover regional settings
No longer blocks: 1333184
Depends on: 1333184
:jfkthame - for this bug, I will want to add:
 - HourCycle
 - DateFormat
 - TimeFormat
 - DateTimeFormat

I will follow your patches.

Those methods will need to be exposed to JS so I expect that we'll want to write it as mozIOSPreferences. I can base it on mozILocaleService, but what I'm not sure is if I should just add the methods on OSPreferences.h/cpp as NS_IMETHODIMPs?

Is it ok for NS_IMETHODIMPs to be targetting both cpp code and js code or should I write regular cpp methods and then NS_IMETHODIMP wrapper functions for those?
Flags: needinfo?(stas) → needinfo?(jfkthame)
In principle, it's fine for C++ code to call the NS_IMETHODs; but depending what the APIs are like, it may sometimes be more convenient to expose underlying C++-only methods as well.

I guess in this case, the IDL will simply have:

    ACString dateFormat(in long dateStyle);

which will then appear in the .cpp file as something like:

    NS_IMETHODIMP
    DateFormat(int32_t aDateStyle, nsACString& aRetVal)
    {
      aRetVal = ...something...
      return NS_OK;
    }

That's fine to use directly from C++, so no need to do anything further.

It's generally when the IDL method is dealing with JS types/objects (e.g. it's declared with [implicit_jscontext] so the caller needs to have a JSContext on hand to pass, or it is expecting/returning a jsval, etc) that it would be cumbersome for non-JS-engine Gecko code to call these methods, and it's helpful to expose a pure-C++ version as well.

(Speaking of which, in mozILocaleService I should've pointed out that there was no need for [implicit_jscontext] on the getAppLocale method, AFAICS, so we can drop that and fold the JS-callable and C++ versions together into a single NS_IMETHOD. I'll post a followup patch there.)
Flags: needinfo?(jfkthame)
This is deeply WIP. I just:

 - added mozIOSPreferences

I was hoping to avoid that, but it seems that one of the first cases of nsLocaleService is Telemetry which actually wants OS language.

 - added event "intl:selected-locales-changed" which we should trigger on OS locale changes

 - added OSPreferences::getTimePattern

based on :jfkthame's code from the previous patches.

It took me a while to get my head wrapped around how to tackle this bug because it seems that GTK/Android behavior is very different from Mac/Windows.

GTK and Android allow you to just decide on 12/24h clock, while Windows allows you to select date/time format, and mac allows you to do both.

In order to achieve the end goal, which is to allow mozIntl.DateTimeFormat to use `style` option that resolves to one of 12 patterns (date|time|datetime*short|medium|long|full) that respect OS preferences, I see two ways:

1) make OSPreferences "smart"

Take ICU's skeleton in OSPreferences::get*Pattern for the given style and modify it using OS preferences.
For example, it could retrieve skeleton "Hm" and replace H->h to get skeleton "hm".

Then, OSPreferences would retrieve the pattern for such skeleton and return it.

Pros:
 - simplifies OSPreferences API, allows per-OS fine-tuning using as many preferences as OS has

Cons:
 - makes OSPreferences do more than just retrieve data from the OS. Now it also retrieves data from CLDR and uses OS prefs to manipulate it

2) make mozIntl.DateTimeFormat "smart"

call OSPreferences::get*Pattern from mozIntl, and if it returns false (could not retrieve pattern from OS), then it would retrieve the skeleton from ICU, call more specific functions from OSPreferences like "GetHourCycle", modify the skeleton and retrieve a pattern from ICU for it.

Pros:
 - keeps OSPreferences simple, the logic is close to the functionality

Const:
 - requires extra methods on OSPreferences which are per-OS ("clock-show-seconds" in gtk etc.) and complicates the OSPreferences API 

:jfkthame, :pike - do you have opinion on what should we do here?
Flags: needinfo?(l10n)
Flags: needinfo?(jfkthame)
Note to self, from looking at the patch the event is called "intl:system-locales-changed", which addresses my immediate comment :-)

Regarding where to put complexity, I'd probably put it in osprefs directly.

Few code comments, put the constants and the function taking it close together?

Also, can systemLocales be a js value returning an array? I think Gijs said something in that direction on the mailing list.
Flags: needinfo?(l10n)
I think I'd lean towards doing the work in OSPreferences, so that it just exposes a getPattern method (that takes parameters for date & time style) or something like that.

In cases where the OS gives us direct access to patterns (IIRC, it does on Mac & Win?), we could just return the pattern that the system provides. It's only on Gtk & Android that getPattern would need to retrieve a skeleton from ICU, customize it according to the bits of info we can get from OS APIs (e.g. 12/24hr, show-seconds, whatever), and then resolve to an ICU pattern.
Flags: needinfo?(jfkthame)
Cool! Glad to see consensus. I'll implement it.

Follow-up style question:

Should I make the API take OSPreferences::TimeFormatSelector::Long type of bits, or is ECMA402 style of taking a string "long" good enough?
On one hand, I feel like C++ prefers the former approach, on the other, JS prefers the latter.

I feel like this API will be very local and used only by mozIntl which is in JS, but I'd also like to keep it consistent with Mozilla internal conventions.
Flags: needinfo?(jfkthame)
I'd go for a set of named constants in the IDL (though I guess I'm biased towards looking at things from a C++ point of view). Just seems like it's more lightweight/efficient than strings, as the methods can do a simple switch() on the parameter value instead of a series of strcmp() or equivalent.
Flags: needinfo?(jfkthame)
NI on :jfkthame to help with ICU+C++ string conversions in my latest attempt to get skeleton+patterns - https://reviewboard.mozilla.org/r/110906/diff/1-2/
Flags: needinfo?(jfkthame)
Here's a version of the patch that builds OK on macOS and Windows, and seems to make mozIOSPreferences.getTimePattern return sensible results from the system on both those platforms. I haven't looked into the Linux/Gtk/Android/etc side of it yet, will go there next...
That works great! I updated the MR patch to use your code.

I tested on mac and windows and it works as expected.

One interesting caveat is that Mac returns the altered values only if you don't pass a locale explicitly.

If I ask for `osp.getTimePattern(osp.timeFormatStyleShort);` it will return format with my modifications. If I pass locale, including the currently used one, it returns defaults for the given locale.
Windows on the other hand, returns format with modifications for when I pass no locale or when the locale matches the current OS locale.

I'll document it and we will have to decide how we want it to work with mozIntl. The question is:

If your OS is in "en-US" and you modified your time format, and Firefox is in a different locale (say, "fr"), do you expect Firefox to follow your regional customizations?
My take is that yes, if I set my time format to h24 that means I want my time format to be h24, no matter what locale.

We can achieve it on win/mac by calling OSP from mozIntl with no locale and using the returned value.
Updated to build and run on Linux, although I'm not sure the gsettings-based HourCycle check in the Gtk code is actually what we want to be checking.
Attachment #8837195 - Attachment is obsolete: true
Oops, I broke the Mac build in the previous version; now fixed.
Attachment #8837681 - Attachment is obsolete: true
Attachment #8837698 - Attachment is obsolete: true
Attachment #8801110 - Attachment is obsolete: true
Attachment #8801110 - Flags: feedback?(gandalf)
Attachment #8801109 - Attachment is obsolete: true
Attachment #8801109 - Flags: feedback?(gandalf)
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
I think this is a good stepping stone.

It does a reasonable effort to support win/mac/gtk for date/time patterns, allowing windows/mac to alter the whole pattern, and respecting user modified h12/h24 in GTK.

There's still work to do to add Ubuntu support, but I expect it to work very similarly to Gtk.
Also, we'll eventually want to add per-os event listener to be able to react to regional settings changes, but I believe we can do that later.
Also, I found how to get the hourcycle in Unity (Ubuntu).

Basically, all we have to do is:

1) Read env XDG_CURRENT_DESKTOP
2) If it's GNOME (or anything else really) just follow gtk
3) If it's Unity, take `com.canonical.indicator.datetime` `time-format` instead of the gnome one.
The unity one can take '12-hour', '24-hour', 'locale-default' or 'custom'. I believe we should only react to 12/24h explicitly stated, and keep using g_settings_get_user_value to make sure that we only take user overrides.

There may be other values used by other environments (mate seems to be using org.mate.panel.objects.clock.prefs.format and I have no idea what elementary os uses) but I'd say that at this point if we cover Gnome and Ubuntu we're doing a good job :)
Comment on attachment 8835209 [details]
Bug 1308329 - Extend OSPreferences API to cover date/time styles.

https://reviewboard.mozilla.org/r/110906/#review114924

::: intl/locale/OSPreferences.cpp:179
(Diff revision 5)
> +  return true;
> +}
> +
> +/**
> + * This function is a counterpart to GetDateTimeSkeletonForStyle.
> + * 

trailing space

::: intl/locale/OSPreferences.cpp:197
(Diff revision 5)
> +  UDateTimePatternGenerator* pg = udatpg_open(PromiseFlatCString(aLocale).get(), &status);
> +  if (U_FAILURE(status)) {
> +    return false;
> +  }
> +
> +  const int32_t kPatternMax = 32;

32 seems too small here; a full date+time pattern could easily exceed this. I'd suggest a buffer size of a couple of hundred chars or so, to be on the safe side.

Alternatively, we could deal with ICU returniing a buffer-too-small status, and reallocate dynamically; but just providing a very-generous buffer up front should be sufficient, I think.

::: intl/locale/OSPreferences.cpp:232
(Diff revision 5)
> +    return false;
> +  }
> +
> +  int32_t resultSize;
> +  const UChar* value = udatpg_getDateTimeFormat(pg, &resultSize);
> +  MOZ_ASSERT(resultSize >= 0);

As a sanity-check, maybe we should verify that the result contains both "{0}" and "{1}", and fall back to something minimal like "{1} {0}" if it doesn't. Or just return false in that case, and let the caller decide what fallback to do.

::: intl/locale/mac/OSPreferences_mac.cpp:87
(Diff revision 5)
> + * Cocoa API maps nicely to out four styles of date/time.
> + * 

s/out/our/, and lose the trailing space on the next line

::: intl/locale/mozIOSPreferences.idl:43
(Diff revision 5)
> +   * The API may return an empty list in case no locale could
> +   * be retrieved from the system.
> +   *
> +   * (See OSPreferences.h for a more C++-friendly version of this.)
> +   */
> +  [implicit_jscontext] jsval getSystemLocales();

In view of bug 1336281 comment 28, I wonder if this should be exposed as something like

    void getSystemLocales([optional] out unsigned long aCount,
                          [retval, array, size_is(aCount)] out DOMString aOutArray);

with corresponding updates to the implementation.

::: intl/locale/windows/OSPreferences_win.cpp:82
(Diff revision 5)
> + * we map it to our four options as:
> + *
> + *   short  -> short
> + *   medium -> short
> + *   long   -> long
> + *   fill   -> long

s/fill/full/

However, I wonder if we should be doing this? It means for Windows, we'll be collapsing the 4 styles exposed in the API into only two results. Maybe we should instead go with CLDR patterns for the "full" style, perhaps customized with the 12/24-hr setting from the OS like in the Gtk case?

::: intl/locale/windows/OSPreferences_win.cpp:106
(Diff revision 5)
> +                aDateStyle != DateTimeFormatStyle::Invalid;
> +  bool isTime = aTimeStyle != DateTimeFormatStyle::None &&
> +                aTimeStyle != DateTimeFormatStyle::Invalid;
> +
> +  if (isDate && isTime) {
> +    GetDateTimeConnectorPattern(aLocale, aRetVal);

We should check for failure here.

::: intl/locale/windows/OSPreferences_win.cpp:110
(Diff revision 5)
> +  if (isDate && isTime) {
> +    GetDateTimeConnectorPattern(aLocale, aRetVal);
> +  }
> +
> +  if (isDate) {
> +    nsAutoString dateString;

unneeded, see below

::: intl/locale/windows/OSPreferences_win.cpp:120
(Diff revision 5)
> +    dateString.Truncate(0);
> +    for (const WCHAR* p = dateBuffer; *p; ++p) {
> +      dateString.Append(char16_t(*p));
> +    }

This could be done as a single .Assign, instead of an explicit loop (unlike the time case below, we're not tweaking the characters as we go).

But I think it's not needed at all, see below.

::: intl/locale/windows/OSPreferences_win.cpp:126
(Diff revision 5)
> +    for (const WCHAR* p = dateBuffer; *p; ++p) {
> +      dateString.Append(char16_t(*p));
> +    }
> +
> +    if (isTime) {
> +      //aRetVal.ReplaceSubstring("{1}", dateString);

You probably wanted

    aRetVal.ReplaceSubstring(u"{1}", dateString);

here.

But wait, we don't need to copy stuff into a local dateString variable at all. I think you can say

    aRetVal.ReplaceSubstring(u"{1}", nsDependentString((const char16_t*)dateBuffer, len));

here and eliminate an entire string-copy.

::: intl/locale/windows/OSPreferences_win.cpp:128
(Diff revision 5)
> +    }
> +
> +    if (isTime) {
> +      //aRetVal.ReplaceSubstring("{1}", dateString);
> +    } else {
> +      aRetVal.Append(dateString);

It should work to do

    aRetVal.Assign((const char16_t*)dateBuffer, len);

here.

::: intl/locale/windows/OSPreferences_win.cpp:148
(Diff revision 5)
> +    // Windows uses "t" or "tt" for a "time marker" (am/pm indicator),
> +    //   https://msdn.microsoft.com/en-us/library/windows/desktop/dd318148(v=vs.85).aspx
> +    // but in a CLDR/ICU-style pattern that should be "a".
> +    //   http://userguide.icu-project.org/formatparse/datetime
> +    // So we fix that up here.
> +    timeString.Truncate(0);

No need to truncate this, it's a local var that starts out empty.

::: intl/locale/windows/OSPreferences_win.cpp:162
(Diff revision 5)
> +        timeString.Append(char16_t(*p));
> +      }
> +    }
> +
> +    if (isDate) {
> +      //aRetVal.ReplaceSubstring("{0}", timeString);

You want `u"{0}"` for the literal Unicode string.

::: intl/locale/windows/OSPreferences_win.cpp:168
(Diff revision 5)
> +    } else {
> +      aRetVal.Append(timeString);
> +    }
> +  }
> +
> +  return true;

Actually, I wonder if we can restructure this slightly, to eliminate more string-copy operations in the case where only date or only time is wanted (so we don't need to do the placeholder-substitution in the date-time pattern). In this case, we should be able to read the pattern from Windows directly into the destination string variable.

Let me see what I can cook up here...
Hahaha, damn strings!!! :)

I couldn't get the "ReplaceSubstring" to work with nsAutoString, maybe because of the params, Idk.

> As a sanity-check, maybe we should verify that the result contains both "{0}" and "{1}", and fall back to something minimal like "{1} {0}" if it doesn't. Or just return false in that case, and let the caller decide what fallback to do.

I would assume that ICU does such a check on its own. Their API guarantees to return a pattern with 0 and 1.

> However, I wonder if we should be doing this? It means for Windows, we'll be collapsing the 4 styles exposed in the API into only two results. Maybe we should instead go with CLDR patterns for the "full" style, perhaps customized with the 12/24-hr setting from the OS like in the Gtk case?

It's a tradeoff for sure, but my POV is that if we go for ICU+modifications we'd end up with some weird mix in cases where the user actually did modify the pattern.

His short/long would be, say "Y/m/d" variants, while his medium/full would be "Y-m-d". I'd like to avoid that.

As you know, I'd like to eventually get a way to check if the user modified the patterns, and if he didn't go for ICU, and if he did, use the modified patterns (both on mac and windows).

But I think that for now, this algo is a good approximation that shades on the side of following what the OS expects us to use.

If you know how to check env variable `XDG_CURRENT_DESKTOP` from inside of the OSPreferences_gtk.cpp, we could add Unity this way. Or if you prefer, we can split intl/locale/unity.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #53)
> Hahaha, damn strings!!! :)
> 
> I couldn't get the "ReplaceSubstring" to work with nsAutoString, maybe
> because of the params, Idk.

Your literal "{1}" was an 8-bit string, but nsAutoString is 16-bit; that would have prevented it compiling. u"{1}" is a 16-bit literal, so AFAIK that should work.

But hold on, I'll post a suggested revision of this shortly (once I get back to my Windows box).

> 
> > As a sanity-check, maybe we should verify that the result contains both "{0}" and "{1}", and fall back to something minimal like "{1} {0}" if it doesn't. Or just return false in that case, and let the caller decide what fallback to do.
> 
> I would assume that ICU does such a check on its own. Their API guarantees
> to return a pattern with 0 and 1.

OK.

> > However, I wonder if we should be doing this? It means for Windows, we'll be collapsing the 4 styles exposed in the API into only two results. Maybe we should instead go with CLDR patterns for the "full" style, perhaps customized with the 12/24-hr setting from the OS like in the Gtk case?
> 
> It's a tradeoff for sure, but my POV is that if we go for ICU+modifications
> we'd end up with some weird mix in cases where the user actually did modify
> the pattern.
> 
> His short/long would be, say "Y/m/d" variants, while his medium/full would
> be "Y-m-d". I'd like to avoid that.
> 
> As you know, I'd like to eventually get a way to check if the user modified
> the patterns, and if he didn't go for ICU, and if he did, use the modified
> patterns (both on mac and windows).
> 
> But I think that for now, this algo is a good approximation that shades on
> the side of following what the OS expects us to use.

Fair enough.

> If you know how to check env variable `XDG_CURRENT_DESKTOP` from inside of
> the OSPreferences_gtk.cpp, we could add Unity this way. Or if you prefer, we
> can split intl/locale/unity.

You should be able to just use getenv() to get the env var and compare it to whatever Unity's value is. Probably cache the result so we don't have to do it every time; I guess the current desktop doesn't change during a session.
Flags: needinfo?(jfkthame)
Something like this should fix up the Windows stuff, I think. Note that there are parts of the Windows date pattern that don't match up with CLDR conventions, so those also need to be adjusted. (I didn't try to add Ubuntu/unity support to the Linux side yet.)
Attachment #8838788 - Flags: feedback?(gandalf)
Comment on attachment 8838788 [details] [diff] [review]
Extend OSPreferences API to cover date/time styles

looks great! thank you

I added it to the full patch, added the Unity check and some basic tests. I think we're ready. Launching try runs :)
Attachment #8838788 - Flags: feedback?(gandalf) → feedback+
Attachment #8838788 - Attachment is obsolete: true
So, I had to give up on the g_settings_get_user_value use, because our test machines use older gtk.

I don't fully understand why, since our test automation is supposed to be using gtk 3 since bug 1186003 and this API has been added in 2.40.

I added a note about our intent to switch to it once we bump the minimum gtk version (I have no idea what it is now).
:jfkthame, do you think it would be worth to file a bug and request the bump? gtk 2.40 is 4 years old. I don't expect many people trying to build latest Firefox that still rely on gtk pre 2.40.
Flags: needinfo?(jfkthame)
Comment on attachment 8835209 [details]
Bug 1308329 - Extend OSPreferences API to cover date/time styles.

https://reviewboard.mozilla.org/r/110906/#review115224

::: intl/locale/gtk/OSPreferences_gtk.cpp:43
(Diff revisions 6 - 7)
> +  bool unity = env != nullptr && strcmp(env, "Unity") == 0;
> +
> +  const char* schema_id = unity ?
> +    "com.canonical.indicator.datetime" : "org.gnome.desktop.interface";
> +  const char* key = unity ? "time-format" : "clock-format";

The previous version of this, with a single if-else, was much more readable IMO. I think all you need to do to fix the compile error it gave is to declare the variables as `const char*` instead of plain `char*`. (And might as well do the same for `env` while we're here.)

(The `const` there doesn't make the variable itself const, such that you can't assign to it; it's saying that the character buffer pointed to by the variable is const, which is fine -- we don't want to modify its contents.)

::: intl/locale/gtk/OSPreferences_gtk.cpp:55
(Diff revisions 6 - 7)
>    if (settings) {
> -    GVariant* value = g_settings_get_user_value(settings, key);
> +    // We really want to use g_settings_get_user_value which will
> +    // only want to take it if user manually changed the value.
> +    // This requires gtk 2.40, so we'll only be able to do this after
> +    // we bump the minimum version.
> +    GVariant* value = g_settings_get_value(settings, key);

IIUC, it's not the gtk version that is relevant here, it's the gio version, which is a separate thing. We currently require 2.22 (see https://dxr.mozilla.org/mozilla-central/rev/d11c29c1db3a1bc96ad5792ebf8a89b2fbadcf85/old-configure.in#60), though I believe gio is now bundled and released as part of glib, for which we require 2.26; we recently fixed an issue where we inadvertently introduced a dependency on 2.32, and broke some people's builds (bug 1324780).

All of which suggests that requiring 2.4 may not yet be feasible; it's just about 3 years old (https://en.wikipedia.org/wiki/GLib#Releases), but older ("more stable") distros may well not have it yet.

What we should be able to do here, though, is check at runtime for the availability of g_settings_get_user_value, and use it if it's present, falling back to g_settings_get_value only if we're running with an older glib. I'll look into that.
(In reply to Jonathan Kew (:jfkthame) from comment #60)
> All of which suggests that requiring 2.4 may not yet be feasible; it's just

s/2.4/2.40/
Flags: needinfo?(jfkthame)
Updated the Gtk code so it builds with older glib, but will use the new get_user_value function if available at runtime. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f776028f7db9af44bfec74cb26b9d27132eae19
Looks great! Added it to the MR patch and retriggered try run.
Comment on attachment 8835209 [details]
Bug 1308329 - Extend OSPreferences API to cover date/time styles.

https://reviewboard.mozilla.org/r/110906/#review116434

This is close to being ready to land, I think - pretty minor updates/tweaks suggested below.

::: intl/locale/OSPreferences.h:146
(Diff revision 8)
> +   * separately for each platform.
> +   *
> +   * It is `best-effort` kind of API that attempts to construct the best
> +   * possible date/time pattern for the given styles and locales.
> +   *
> +   * The heuristic may depend on the OS API and HIG guidelines.

Add a note to make it explicit that this may return `false`, indicating that we failed (or don't know how on this platform) to get a pattern, and the caller needs to decide what to do instead.

::: intl/locale/OSPreferences.cpp:305
(Diff revision 8)
> +  }
> +  DateTimeFormatStyle timeStyle = ToDateTimeFormatStyle(aTimeFormatStyle);
> +  if (timeStyle == DateTimeFormatStyle::Invalid) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +  ReadDateTimePattern(dateStyle, timeStyle, aLocale, aRetVal);

We should check the return value of ReadDateTimePattern here, and return some kind of error if it fails (I guess just NS_ERROR_FAILURE if nothing more specific seems appropriate).

::: intl/locale/gtk/OSPreferences_gtk.cpp:42
(Diff revision 8)
> +typedef GVariant* (*get_value_fn_t)(GSettings*, const gchar*);
> +
> +static get_value_fn_t
> +FindGetValueFunction()
> +{
> +  get_value_fn_t fn = reinterpret_cast<get_value_fn_t>(dlsym(RTLD_DEFAULT, "g_settings_get_user_value"));   

Very long line, please wrap it. And lose the trailing space.

(Yes, I know this came from my patch. So please fix my bad style!)

::: intl/locale/mac/moz.build:10
(Diff revision 8)
> +    'OSPreferences_mac.cpp'
>  ]
>  
> -if CONFIG['ENABLE_INTL_API']:
> -    UNIFIED_SOURCES += ['OSPreferences_mac.cpp']
> -

I think the removal of ENABLE_INTL_API checks from existing code should go in a separate bug, not be mixed in with the new stuff that's landing here.

(There's another of these later, too.)

::: intl/locale/tests/gtest/TestOSPreferences.cpp:36
(Diff revision 8)
> +  OSPreferences::GetInstance()->GetDateTimePattern(0, 0, NS_LITERAL_CSTRING(""), pattern);
> +
> +  OSPreferences::GetInstance()->GetDateTimePattern(1, 0, NS_LITERAL_CSTRING("pl"), pattern);
> +  OSPreferences::GetInstance()->GetDateTimePattern(2, 0, NS_LITERAL_CSTRING("de-DE"), pattern);
> +  OSPreferences::GetInstance()->GetDateTimePattern(3, 0, NS_LITERAL_CSTRING("fr"), pattern);
> +  OSPreferences::GetInstance()->GetDateTimePattern(4, 0, NS_LITERAL_CSTRING("ar"), pattern);
> +
> +  OSPreferences::GetInstance()->GetDateTimePattern(0, 1, NS_LITERAL_CSTRING(""), pattern);
> +  OSPreferences::GetInstance()->GetDateTimePattern(0, 2, NS_LITERAL_CSTRING("it"), pattern);
> +  OSPreferences::GetInstance()->GetDateTimePattern(0, 3, NS_LITERAL_CSTRING(""), pattern);
> +  OSPreferences::GetInstance()->GetDateTimePattern(0, 4, NS_LITERAL_CSTRING("ru"), pattern);
> +
> +  OSPreferences::GetInstance()->GetDateTimePattern(4, 1, NS_LITERAL_CSTRING(""), pattern);
> +  OSPreferences::GetInstance()->GetDateTimePattern(3, 2, NS_LITERAL_CSTRING("cs"), pattern);
> +  OSPreferences::GetInstance()->GetDateTimePattern(2, 3, NS_LITERAL_CSTRING(""), pattern);
> +  OSPreferences::GetInstance()->GetDateTimePattern(1, 4, NS_LITERAL_CSTRING("ja"), pattern);

Do

    OSPreferences* osprefs = OSPreferences::GetInstance();

at the beginning, so that you can abbreviate these lines to `osprefs->Get...`.

Actually, I think we should also test that we get a non-empty pattern in each case (except with 0, 0) if the method succeeds.

I'd also be inclined to put the test params into an array and then loop over it, something like:

    struct Test {
      int dateStyle;
      int timeStyle;
      const char* locale;
    };
    Test tests[] = {
      { 0, 0, "" },
      { 1, 0, "pl" },
      ...etc
    };

    for (unsigned i = 0; i < ArrayLength(tests); i++) {
      const Test& t = tests[i];
      nsAutoString pattern;
      if (NS_SUCCEEDED(inst->GetDateTimePattern(t.dateStyle, t.timeStyle,
                                                nsDependentCString(t.locale),
                                                pattern)) {
        ASSERT_TRUE((t.dateStyle == 0 && t.timeStyle == 0) || !pattern.IsEmpty());
      }
    }

(untested code written in mozreview, use at your own risk!)

::: intl/locale/tests/unit/test_osPreferences.js:6
(Diff revision 8)
> + * Make sure the locale service can be instantiated,
> + * and returns something plausible for getAppLocale and getAppLocales.

This comment looks copied from a different file. :)

::: intl/locale/tests/unit/test_osPreferences.js:25
(Diff revision 8)
> +  osprefs.getDateTimePattern(
> +    osprefs.dateTimeFormatStyleNone, osprefs.dateTimeFormatStyleNone, "");
> +
> +  osprefs.getDateTimePattern(
> +    osprefs.dateTimeFormatStyleShort, osprefs.dateTimeFormatStyleNone, "");
> +
> +  osprefs.getDateTimePattern(
> +    osprefs.dateTimeFormatStyleNone, osprefs.dateTimeFormatStyleLong, "ar");
> +
> +  osprefs.getDateTimePattern(
> +    osprefs.dateTimeFormatStyleFull, osprefs.dateTimeFormatStyleMedium, "ru");

A bit like the gtest, I think we should check that the result is non-empty unless the params were both None; and shouldn't we wrap getDateTimePattern with a try/catch because it could fail?
I applied your feedback, thanks!

> We should check the return value of ReadDateTimePattern here, and return some kind of error if it fails (I guess just NS_ERROR_FAILURE if nothing more specific seems appropriate).

Except of this bit. I decided not to make the method in JS throw in case we can't retrieve a pattern, because it doesn't feel like the right API for JS.

In JS, we return a string. If the string is empty, we can't use it, so the user has to check if the returned pattern is empty anyway. There doesn't seem to be much value to on top of that require users to wrap each call in try/catch in case it'll throw somewhere and I'd be worried about users not doing it and then the error being thrown only in edge cases on esoteric platform combinations.

So instead, I just assume that in an error case the API will return an empty string.

Let me know if you're ok with that.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #69)
> I applied your feedback, thanks!
> 
> > We should check the return value of ReadDateTimePattern here, and return some kind of error if it fails (I guess just NS_ERROR_FAILURE if nothing more specific seems appropriate).
> 
> Except of this bit. I decided not to make the method in JS throw in case we
> can't retrieve a pattern, because it doesn't feel like the right API for JS.
> 
> In JS, we return a string. If the string is empty, we can't use it, so the
> user has to check if the returned pattern is empty anyway. There doesn't
> seem to be much value to on top of that require users to wrap each call in
> try/catch in case it'll throw somewhere and I'd be worried about users not
> doing it and then the error being thrown only in edge cases on esoteric
> platform combinations.

I'm worried about users just assuming the call will return a usable pattern, and not checking it for empty and providing a fallback of some kind.

On thinking again about this, I wonder if the fallback to ICU/CLDR patterns should actually be implemented right here in OSPreferences, so that it -never- returns an empty pattern? On an OS where we don't know how to check any user prefs, we'd simply return the pattern as-is. After all, that's -almost- what we're doing in the Gtk code here; we're returning a pattern which is 90% direct from ICU, with just a minor tweak based on a single user pref. So why not do it 100% for the generic case?

> 
> So instead, I just assume that in an error case the API will return an empty
> string.
> 
> Let me know if you're ok with that.

Hmmm. Maybe what we should do here is make a clearer distinction between "there's no pattern to return", in which case we should leave the string empty but not throw an error, and "something unexpectedly failed in here", in which case throwing an error is probably the right thing to do.

This translates, I think, to making the OSPreferences_unix version of ReadDateTimePattern simply return 'true' (nothing went unexpectedly wrong) but leave the return value empty (we don't have patterns there). A 'false' result from ReadDateTimePattern would be reserved for an actual failure in a case that's supposed to be supported, not for the case of empty-because-we-don't-know-any-better.

(But see above; on second thoughts, I'm not sure this is the most useful approach.)

The other toolkit-specific implementations all have code paths that can in theory return 'false', but that should only happen in extreme failure situations (such as out-of-memory), and in that case I think throwing NS_ERROR_FAILURE is appropriate.

(The immediate caller wouldn't necessarily need to catch that; in the worst case it could terminate the script entirely, which is the proper response to an internal resource-allocation failure or similar, if the caller doesn't explicitly choose to handle and recover from it.)

Does that make sense?

Oh, that does mean that the test for a non-empty pattern result (in the gtest) won't hold true for the generic unix version. So I guess the test there should instead just be asserting that the call succeeds.

Unless (as I'm starting to think, see above) we fall back to the unmodified ICU pattern in the case where we don't know any better.
> On thinking again about this, I wonder if the fallback to ICU/CLDR patterns should actually be implemented right here in OSPreferences, so that it -never- returns an empty pattern? On an OS where we don't know how to check any user prefs, we'd simply return the pattern as-is. After all, that's -almost- what we're doing in the Gtk code here; we're returning a pattern which is 90% direct from ICU, with just a minor tweak based on a single user pref. So why not do it 100% for the generic case?

This is an option, yes. I was slightly uncomfortable with that originally because separation of concerns would dictate, that OSPreferences just do the reading from the OS, and mozIntl does the logic to cover when things go right/wrong to produce the pattern.

But since the *logic* is also platform specific, it seems reasonable to read the pattern per platform.

That leaves this grey area where a platform may not read anything, and have no customizations, but maybe it should just return ICU's value.

I'd like to minimize the amount of this kinds of APIs in OSPreferences and where possible keep it dumbed down to "ready from OS, react to OS events", but for this one it may make sense to go all the way instead of half of the way.

I'll update the patch.
Attachment #8838876 - Attachment is obsolete: true
I updated the patch to pull the ICU pattern for the style if Read* returns false.
Is there a lot left for your review :jfkthame?
Flags: needinfo?(jfkthame)
Comment on attachment 8835209 [details]
Bug 1308329 - Extend OSPreferences API to cover date/time styles.

https://reviewboard.mozilla.org/r/110906/#review117918

OK, I think we're ready to go here; just a couple little cosmetic things that could be tidied up.

::: intl/locale/OSPreferences.cpp:333
(Diff revision 12)
> +
> +  if (!ReadDateTimePattern(dateStyle, timeStyle, aLocale, aRetVal)) {
> +    if (!GetDateTimePatternForStyle(dateStyle, timeStyle, aLocale, aRetVal)) {
> +      return NS_ERROR_FAILURE;
> +    }
> +

lose the blank line

::: intl/locale/mozIOSPreferences.idl:67
(Diff revision 12)
> +   * preferences.
> +   *
> +   * Notice, that depending on the OS it may take into account those settings
> +   * for all locales, or only if the locale matches the OS locale.
> +   *
> +   * It takes two arguments of dateTimeFormatStyle* type, and a string

The way this comment is written seems a little confusing, as `dateTimeFormatStyle` isn't actually a type here; the args are just integers. Maybe "It takes two integer arguments that must be valid `dateTimeFormatStyle*` values (see constants defined above), and a string..."

::: intl/locale/tests/gtest/TestOSPreferences.cpp:66
(Diff revision 12)
> +                                              nsDependentCString(t.locale),
> +                                              pattern))) {

fix indent to align the arguments, please
Attachment #8835209 - Flags: review?(jfkthame) → review+
Hmm, looks like tryserver has a couple of test failures (X on Win & Mac), so we'll need to look into those - either we're not getting the results we expect, or the test is making a bad assumption?
Flags: needinfo?(jfkthame)
Oh, the failure is just because test_osPreferences.js checks that the pattern is non-empty, but includes a test case with date=None, time=None, for which an empty pattern is expected.

So change the test to something like

    do_check_true(pattern.length > 0 || (test[0] == 0 && test[1] == 0),
                  "pattern is not empty.");

and it should be happier.

Also, the fact that we *didn't* get the same failure on Linux surprises me. It turns out that the Linux version of getDateTimePattern, when given None, None as arguments, actually returns something like "yyyyMMdd hh:mm a". That seems wrong. If either one of the args is None, the corresponding part of the pattern is omitted, as expected. But if they're both None, we get....this?
yeah, I can reproduce it. It seems that GetDateTimeSkeletonForStyle returns a non-empty skeleton for NONE/NONE. I'll look into this.

Thanks!
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0fb088fb7d66
Extend OSPreferences API to cover date/time styles. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/0fb088fb7d66
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1344596
Blocks: 1344594
Depends on: 1344038
Sounds like "OS=ALL"
OS: Unspecified → Other
Depends on: 1356718
Regressions: 1510325
You need to log in before you can comment on or make changes to this bug.