Closed Bug 1301640 Opened 8 years ago Closed 8 years ago

Implement nsIDateTimeFormat on top of ICU

Categories

(Core :: Internationalization, defect, P3)

Product:
Core
Component:
Internationalization
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: hsivonen, Assigned: gmoore)

References

Details

(Keywords: good-first-bug)

Attachments

(2 files, 15 obsolete files)

Output from running the gtest file.
570 bytes, text/plain
Details
Fixed small formatting error in nsNSSCertValidity.cpp introduced by previous patch.
1.41 KB, patch
emk
: review+
Details | Diff | Splinter Review 
To have less code (we now have per-platform variants) and to have more correct code (nsDateTimeFormatUnix.cpp calls C standard library setlocale, which affects the behavior of the C standard library for all threads! [The i18n design of the C standard library is fundamentally broken.]), we should implement nsIDateTimeFormat on top of ICU's UDateFormat / udat.h.

It would make sense to simplify the interface while at it. Observations:

 * All C++ callers except the scriptability wrapper pass nullptr as the locale argument to the methods. All the in-tree non-test-case JS callers pass "" as the locale to the scriptability wrapper. We should remove the ability to pass in the locale and write code only for the "current browser UI locale" case.

 * FormatTime is only called by the scriptablility wrapper: https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsIDateTimeFormat%3A%3AFormatTime%28nsILocale+%2A%2C+const+nsDateFormatSelector%2C+const+nsTimeFormatSelector%2C+const+time_t%2C+nsAString_internal+%26%29%22

 * FormatTMTime has no callers: https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsIDateTimeFormat%3A%3AFormatTMTime%28nsILocale+%2A%2C+const+nsDateFormatSelector%2C+const+nsTimeFormatSelector%2C+const+struct+tm+%2A%2C+nsAString_internal+%26%29%22
Oh, and we should deCOMtaminate the whole thing while at it, and provide static C++ methods for the C++ callers.
See Also: → 1301655
We should get rid of the scriptability wrapper, too: bug 1301655.
Keywords: good-first-bug
Another observation: We aren't using all the format selectors available, so a new implementation could reduce the number of selectors supported.
Hi, I am interested in working on this bug. If there's any information that would be helpful for working on it, could you please let me know? Thanks. This would technically be my second bug.
I suggest looking at the existing callers (other than nsScriptableDateFormat) and developing an ICU udat.h-backed class called mozilla::DateTimeFormat that has static methods for the use cases seen in the existing callers of nsIDateTimeFormat. (That is, the methods shouldn't take a locale argument but should assume the UI locale of Firefox.) I suggest placing the code in intl/locale/.

See https://ssl.icu-project.org/apiref/icu4c/udat_8h.html for the ICU API.

See https://dxr.mozilla.org/mozilla-central/source/dom/encoding/FallbackEncoding.cpp#68 for how to obtain the UI locale.
Okay cool, thanks for the information. So from my understanding right now we have a separate class inheriting from nsIDateTimeFormat for each of the 3 different OS's (e.g. nsDateTimeFormatUnix). Each of these classes has the same functionality but uses different OS-dependent function calls. The functionality is taking in some kind of time class / struct and returning a formatted string representing the time. So instead of this we want to have a single static class that has the same functions, but instead uses the portable ICU library. Let me know if this isn't right.

Also, you mentioned we should deCOMtaminate the whole thing. What exactly does this mean / entail? Thanks.
One thing that the current code *might* be doing (not sure if I read it correctly) is that it retrieves from the OS user's preference for hour12/24 clock.

ICU only gives us what the default preference for the locale is, but what we also need is ability to know is if the user overridden this setting and that's per-OS.

I'm currently planning on designing an API based on the proposal from ECMA402 discussion to have `navigator.locales` store locale codes with the hour12/24 information*, but I need to have per-platform code to retrieve that info from the OS.

If this code is doing it, and we'll replace it with ICU then we'll have to re-add it somewhere else and then pull it for the DOM API.


*) sth like `navigator.locales == ['en-US-u-hc-h12', 'fr'-u-hc-h12']`
Hmmm, okay. Should I continue working on this then, or would changing the code to use ICU be unnecessary because we'll still need to do some stuff per OS? Thanks.
I'd say yes. nsIDateTimeFormat should not be OS-dependent. It should, on the other hand, take user preferences into account, very much like how Intl's API does.

And so we need an API that can retrieve those preferences, and that's OS-specific and then allow the consumer of nsIDateTimeFormat choose if they want to use the defaults (and then use navigator.languages style list), or take user preferences into account (and then use navigator.locales style list).
Putting more thinking into this.

Since we're talking about C++ API, maybe we'd like to turn nsIDateTimeFormat into an API that does everything using ICU plus it has a method to retrieve date/time related user preferences from the OS?

Sth like:

nsIDateTimeFormat {
  NS_IMETHOD FormatTime
  NS_IMETHOD GetOSSettings
}

where the latter would currently only return the hour12/hour24.

This way:

a) It can fulfill :hsivonen's proposal from comment 0 and take no locale argument and just passes the user UI language fallback chain with either:

 - hour12 argument set to the result of GetOSSettings
 - locale chain transformed to add `-u-hc-h12` or `-u-hc-h24` extension keys.

b) Allow us to create a simple `navigator.locales` getter that would be taking `navigator.languages` chain and adding the extension keys.

How does it sound?
Filed bug 1303579 for `navigator.locales`.
Okay, cool. I saw what you said earlier and have actually been working on this for a while. Sorry for taking a while to reply.

Also, not sure if you were asking me, but yeah, your idea sounds pretty good to me.
great!

:hsivonen - does it sound good to you as well?

Gregory - does it mean you're taking this bug? :)
Flags: needinfo?(olucafont6)
Flags: needinfo?(hsivonen)
Yeah, I would like to take it if that's possible. Thanks.
Flags: needinfo?(olucafont6)
Totally! Thanks! :)
The main reason I want to do this is to stop calling into the C standard library for locale-sensitive stuff, since that part of the C standard library is not thread-safe. How can we figure out if the system settings use 24-hour clock on Linux without calling into the C standard library?

Note that OS-level date time formatting preferences allow customization way beyond 12h vs. 24h. For example, one could customize OS-level date formatting to use ISO dates (YYYY-MM-DD) even if it's not the locale convention, etc. Do we currently honor that kind of thing on Windows and Mac?

While in principle we should obey OS settings, considering how peripheral date formatting is in our UI, I'd be OK with overlooking OS settings and going with whatever ICU does given a locale.

But as noted, my main goal is getting rid of the C standard library calls on non-Windows/Mac platforms.
Flags: needinfo?(hsivonen) → needinfo?(gandalf)
(In reply to Henri Sivonen (:hsivonen) from comment #16)
> The main reason I want to do this is to stop calling into the C standard
> library for locale-sensitive stuff, since that part of the C standard
> library is not thread-safe. How can we figure out if the system settings use
> 24-hour clock on Linux without calling into the C standard library?

Ugh, so it seems that this is more complicated.

The code used currenty in per-OS library (at least for Linux) is not really looking into user preferences for the OS. It just checks what the default format is for a given locale - and this is a bit we already have in ICU, so yeah, we should be able to remove it.

> Note that OS-level date time formatting preferences allow customization way
> beyond 12h vs. 24h. For example, one could customize OS-level date
> formatting to use ISO dates (YYYY-MM-DD) even if it's not the locale
> convention, etc. Do we currently honor that kind of thing on Windows and Mac?

We don't honor this, seems like we don't honor anything really.

> While in principle we should obey OS settings, considering how peripheral
> date formatting is in our UI, I'd be OK with overlooking OS settings and
> going with whatever ICU does given a locale.

Except that it's going to change. With the emerge of HTML5 date/time form widgets, users will start seeing time pickers.

> But as noted, my main goal is getting rid of the C standard library calls on
> non-Windows/Mac platforms.

Agree.

So, at the moment we have per-OS files that don't do anything ICU cannot replace. Which means that removing the per-OS code is fine.

Also, seems to me like we should only need the C++ API here, because all JS API should go through Intl.DateTimeFormat public API.

At the same time, we will need per-OS API to retrieve user preferences, but that's probably material for separate bug(s).

Does it sound right?
Flags: needinfo?(gandalf) → needinfo?(hsivonen)
I found a very old bug 460988 that seems to touch on this topic, but I don't understand why it claims that the code should make Gecko follow OS preferences, since nothing in this code seems to even touch OS preferences - just locale defaults (so, en-US user who specifies 24h clock will still get 12h clock).

Am I missing something?
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #17)
> Ugh, so it seems that this is more complicated.

Yes. :-(

> > Note that OS-level date time formatting preferences allow customization way
> > beyond 12h vs. 24h. For example, one could customize OS-level date
> > formatting to use ISO dates (YYYY-MM-DD) even if it's not the locale
> > convention, etc. Do we currently honor that kind of thing on Windows and Mac?
> 
> We don't honor this, seems like we don't honor anything really.

I just tested on Windows. Windows is set to en-US and Firefox is en-US. I customized Windows-level short date formatting to use YYYY-MM-DD. Then I launched Firefox and showed the Most Recent Visit in the history viewer. I got dates in the YYYY-MM-DD format.

> > While in principle we should obey OS settings, considering how peripheral
> > date formatting is in our UI, I'd be OK with overlooking OS settings and
> > going with whatever ICU does given a locale.
> 
> Except that it's going to change. With the emerge of HTML5 date/time form
> widgets, users will start seeing time pickers.

Are those going to do date formatting via C++ or via the ECMAScript Intl API?

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #18)
> I found a very old bug 460988 that seems to touch on this topic, but I don't
> understand why it claims that the code should make Gecko follow OS
> preferences, since nothing in this code seems to even touch OS preferences -
> just locale defaults (so, en-US user who specifies 24h clock will still get
> 12h clock).
> 
> Am I missing something?

At least on Windows, the system API is used in such a way that Firefox doesn't need to read the system prefs explicitly, because the Windows date formatting API does so.

It seems to me that it shouldn't be up to Gecko to attempt to map OS-level prefs to ICU. I think one of the following options make sense:
 1) Letting ICU figure out how to map the OS settings to its own formatting option. Does ICU know how to do this? (A quick code search suggests not.)
 2) Using ICU only on non-Windows/Mac platforms in order to avoid regressing OS-level pref adherence on Windows (and Mac?). This won't be of much use if the upcoming date picker widgets will go through the ECMAScript Intl API and if that API will be exclusively ICU-backed.
 3) Using ICU and not honoring OS prefs, since short date formatting is so buried in the UI anyway. (E.g. the download manager and the certificate viewer use longer formatting with month as text. We shouldn't be designing for comm-central use cases anymore.)
 4) Using the C standard library on non-Mac/Windows but not attempting to ever set the locale. After all, the thread safety problem is with setting the locale and our concern is getting the system-wide behavior, not what we've set. Luckily, it seems that code that we have fewer instances of setting the C standard library locale in the codebase than we used to, so this might actually be doable now!

That is, option #4 would mean not using ICU but just removing the ability to set a non-default locale in the nsIDateTimeFormat implementations.

Thoughts?
Flags: needinfo?(hsivonen) → needinfo?(gandalf)
> Are those going to do date formatting via C++ or via the ECMAScript Intl API?

In JS. And we need to design a JS API that will provide that information. That's bug 1303579.
But we need a C++ code that will collect this per-platform information and feed to JS API :)

We're also working in ECMA 402 on standardization of that JS API part.

> At least on Windows, the system API is used in such a way that Firefox doesn't need to read the system prefs explicitly, because the Windows date formatting API does so.

We're moving away from Windows date formatting API. So instead we'd like to read the system prefs.

> It seems to me that it shouldn't be up to Gecko to attempt to map OS-level prefs to ICU

I disagree. In ECMA402 we have this approach of providing building blocks that people can use. Exposing hour12/24 building block provides developers ability to create any clock/time widgets that follow user-selected convention.
Implicitly feeding this through ICU makes it harder.

>  3) Using ICU and not honoring OS prefs, since short date formatting is so buried in the UI anyway. (E.g. the download manager and the certificate viewer use longer formatting with month as text. We shouldn't be designing for comm-central use cases anymore.)

That's what we may want to do for now.

Wrt. hour12/24 vs custom date/time pattern - My experience from work on FxOS is that users are significantly more opinionated and there are many factors of magnitude more users who change hour12/24 setting than set custom date format.

Because of that I believe it's worth focusing on hour12/hour24 OS perf for now.

But once we get to custom date formats, we will want to do similar thing: expose the building block and make Intl JS API understand it.

For example:

let pattern = Intl.getUserSettings().shortDateFormatPattern;

let dtf = new Intl.DateTimeFormat(navigator.locales, {
  pattern: pattern
});
let str = dtf.format(date);
console.log(str);

====

Coming back to the current situation.

I believe we should switch to ICU here and align nsIDateTimeFormat with JS Intl.DateTimeFormat API as much as possible.

Separately we should have a per-OS dateTime related code that retrieves user-defined custom settings. Initial setting that I'd like to retrieve is hour12/hour24, but as you said, there will be more.

Does it sound like a good plan to you?
Flags: needinfo?(gandalf) → needinfo?(hsivonen)
Actually, for the HTML5 time/date form widgets we will need a way to retrieve a date time pattern for the placeholder!

Sooo, we'll do the very same thing:
1) write an API to retrieve the best match for the locale from ICU
2) write an API that retrieves the OS preference

Use the OS preference is present, otherwise use ICU provided data.

That's exactly the same thing as for hour12/24.

So our API for OS preference will be needed to retrieve those two os-specific bits. I'm ok with not blocking on it and regressing a bit here, but I'd like us to agree that we will need an OS-specific API that will retrieve data from the OS.

On the spec level, we'll make ECMA402 API's accept bits from the user and DOM APIs that make the browser retrieve preferences from the OS (and feed them to ECMA402 APIs).
In other words, I'd like to decouple what nsIDateTimeFormat does into two bits:

1) ICU powered DateTimeFormat API that is locale-aware and accepts options that allow user to override the locale-specific defaults. Make it as close to JS Intl.DateTimeFormat as possible.
2) DOM API that retrieves OS specific preferences that allow the user to feed those preferences to (1)

One more thing to notice is that we're moving Firefox UI to L20n and that means that all UI will go through JS and use ECMA402 Intl.DateTimeFormat.
That's why I'm so focused on JS APIs for that and C++ bits only to feed into the JS APIs, and where we leave C++ APIs, I want them as close to JS API as possible.
Flags: needinfo?(jfkthame)
I think what gandalf is proposing in the latest comments above makes good sense. For both Firefox UI and web content, we'll be using Intl.DateTimeFormat (backed by ICU), and for C++ callers, we should have a similar API (though perhaps only part of it -- I'm not sure how much we actually need from C++) backed by the exact same ICU functionality.

The only platform-specific code we'll need will then be (2) above, which will need to read OS preferences but (even if we have to implement it with the C std lib on Linux) shouldn't need to do any thread-unsafe changing of the locale. Any formatting we want to do with a non-system locale will be done entirely through ICU.
Flags: needinfo?(jfkthame)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #21)
> So our API for OS preference will be needed to retrieve those two
> os-specific bits. I'm ok with not blocking on it and regressing a bit here,
> but I'd like us to agree that we will need an OS-specific API that will
> retrieve data from the OS.

Makes sense, but:

 1) Exposing OS-level datetime formatting customizations to the Web is a privacy problem. (Out of scope for this bug; see bug 1303579 comment 2.)

 2) Do our tier-1 systems expose the formatting prefs via an API or do you plan to infer the prefs by asking the system to format some known dates and times?

Anyway, neither point should block this bug, it seems.
Flags: needinfo?(hsivonen)
Agree, in this bug we can focus on migrating the C++ bindings to use ICU and be more aligned with our JS API.
I moved the os pref to bug 1308329.

    
    

    
  

      
      
      
      

        Attached file
          
        Unfinished version of DateTimeFormat.h (obsolete)
        — 
      

    


  

    
    

    
  
Hi, I have been working on this for a while and have written a version of the code that I think is fairly close to being done. I have attached the two files if you want to see them (there's a lot of comments in them that you can probably ignore). I haven't actually tried compiling it yet or testing it or anything, and have just been writing it in a text editor, so I'm pretty sure there's at least a few problems. If you see any please let me know. 

I have a few questions about it so far.
1) Any tips for adding files to the build process? I will start looking into it but I haven't done it before so any help would be good. I think I will just try to add it in addition to the existing nsIDateTimeFormat files for now (without removing any files).
2) Is there a simple way to test the functions I'm writing? I mainly just want to pass in a date/time object to the old code and the new code and make sure I get the same string as output. Would I have to add automated tests that run along with all of the other tests?
3) In comment 4 hsivonen linked to some code on how to obtain the browser UI locale. Is this different then getting the application locale? In the existing per-OS code we get the locale like this:
https://dxr.mozilla.org/mozilla-central/source/intl/locale/mac/nsDateTimeFormatMac.cpp#45
I used the code you linked to in the files I wrote, but just wanted to make sure there's a difference.

Let me know, thanks.
Flags: needinfo?(hsivonen)
Flags: needinfo?(gandalf)

    
    

    
  
Unfortunately, my C++ skills do not qualify me to answer your questions. Sorry!
Flags: needinfo?(gandalf)

    
    

    
  
Okay, maybe hsivonen will be able to help me out. Thanks anyway.

Also, I think I figured out my first question (1). I just added the two file names to intl/locale/moz.build and then did "./mach build intl/locale". It seems to be compiling the two files now and I'm working through the compiler errors. Thanks.

    
    

    
  
(In reply to Gregory Moore from comment #29)
> 1) Any tips for adding files to the build process? I will start looking into
> it but I haven't done it before so any help would be good. I think I will
> just try to add it in addition to the existing nsIDateTimeFormat files for
> now (without removing any files).

The .cpp file needs to be declared in the UNIFIED_SOURCES list of intl/locale/moz.build. The .h file needs to be declared in EXPORTS so that it can be #included by code residing in other directories.

> 2) Is there a simple way to test the functions I'm writing? I mainly just
> want to pass in a date/time object to the old code and the new code and make
> sure I get the same string as output. Would I have to add automated tests
> that run along with all of the other tests?

Please see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/GTest for documentation about and netwerk/test/gtest/TestStandardURL.cpp for an example about C++ unit tests. The new test should reside in intl/locale/tests/gtest/, which doesn't currently exist.

> 3) In comment 4 hsivonen linked to some code on how to obtain the browser UI
> locale. Is this different then getting the application locale? In the
> existing per-OS code we get the locale like this:
> https://dxr.mozilla.org/mozilla-central/source/intl/locale/mac/
> nsDateTimeFormatMac.cpp#45
> I used the code you linked to in the files I wrote, but just wanted to make
> sure there's a difference.

Good point! The two are indeed different. What I pointed to in comment 5 obtains the language of Firefox's UI strings. The code you are pointing to obtains the preferred locale from the operating system. Contrary to what I said in comment 5, the latter should be used to avoid a larger departure from the previous behavior (Firefox trying to follow the OS-level setting for datetime formatting).
Flags: needinfo?(hsivonen)

    
    

    
  
> Contrary to what I said in comment 5, the latter should be used to avoid a larger departure from the previous behavior (Firefox trying to follow the OS-level setting for datetime formatting).

With the switch to L20n we will start using `navigator.languages` (and later `navigator.locales`) for UI locale and Intl APIs.

    
    

    
  
Cool, thanks for all of the information. I did what hsivonen said and was able to add a gtest file that makes sure the new code works and compares its output to the old code. I attached a text file showing the output. The time I passed in was time_t time = 0. I ran it on Windows 7 and Windows 10, both with the exact same results.

From the output it looks like it's working pretty well. There are some differences in the output; for example kTimeFormatSecondsForce24Hour and kTimeFormatNoSecondsForce24Hour don't match the old code (I guess the default settings on Windows will add the AM/PM even if you force 24 hour settings). Also kDateFormatLong and kDateFormatShort don't match the old code either (ICU doesn't print the weekday in the long format and only uses two year digits in the short format). I don't think it can really be perfect though unless we use the OS preferences. I think the ICU output will match the Mac and Linux output better, as well. 

I have a few more questions about the code:

1) hsivonen said in comment 1 that we should deCOMtaminate the code. I'm still not exactly sure what this means. Does this mean we should remove all uses of the nsCOMPtr class? If so, should we replace it with another smart pointer class? Any other parts of the code that need to be deCOMtaminated?

2) hsivonen said in comment 3 that we could reduce the number of supported format selectors. I believe kDateFormatYearMonth, kDateFormatWeekday, kTimeFormatSecondsForce24Hour, and kTimeFormatNoSecondsForce24Hour are never used. Should I just remove all code for those selectors, or should I leave it in, as people may want to use those formats in the future?

3) The time date type that ICU uses is UDate, which represents the number of milliseconds since 1970-jan-01, 00:00 UTC. This is really similar to PRTime and time_t, except for PRTime is in microseconds and time_t is seconds. In the original code the majority of the logic was in FormatTMTime, as they used a tm struct and then if needed, converted it to the appropriate Windows or Mac time struct. It seems like there are 3 options for the new code:
a) Put all of the logic in FormatTime (which takes a time_t), then multiply the given time_t by 1000 to get the corresponding UDate value.  
b) Put all of the logic in FormatPRTime (which takes a PRTime), and then divide the given PRTime by 1000 to get the corresponding UDate value (not sure if just dividing it would work correctly).
c) Have a fifth format function (FormatUDate or something) that takes a UDate value. Then in FormatTime we could multiply the time_t by 1000 and call FormatUDate. In FormatPRTime we could divide the time_t by 1000 and call FormatUDate.
Right now I'm doing option (a). It seems like that's fine because we only print at a precision of seconds, so losing any information about milliseconds is okay. I'm wondering if doing something like option (c) would be better though, as when we call FormatPRTime it calls FormatPRExplodedTime, which calls FormatTMTime, which then finally calls FormatTime. It seems like a lot of variable conversion that might be unnecessary. 
Also, if were going to get rid of FormatTime and FormatTMTime because they don't really have any callers, I guess we would need to do something like option (b) because we wouldn't need either of those functions for conversion.

Let me know, thanks.
Flags: needinfo?(hsivonen)

    
    

    
  
(In reply to Gregory Moore from comment #35)
> Cool, thanks for all of the information. I did what hsivonen said and was
> able to add a gtest file that makes sure the new code works and compares its
> output to the old code. I attached a text file showing the output. The time
> I passed in was time_t time = 0. I ran it on Windows 7 and Windows 10, both
> with the exact same results.
> 
> From the output it looks like it's working pretty well. There are some
> differences in the output; for example kTimeFormatSecondsForce24Hour and
> kTimeFormatNoSecondsForce24Hour don't match the old code (I guess the
> default settings on Windows will add the AM/PM even if you force 24 hour
> settings). Also kDateFormatLong and kDateFormatShort don't match the old
> code either (ICU doesn't print the weekday in the long format and only uses
> two year digits in the short format). I don't think it can really be perfect
> though unless we use the OS preferences. I think the ICU output will match
> the Mac and Linux output better, as well. 
> 
> I have a few more questions about the code:
> 
> 1) hsivonen said in comment 1 that we should deCOMtaminate the code. I'm
> still not exactly sure what this means. Does this mean we should remove all
> uses of the nsCOMPtr class? If so, should we replace it with another smart
> pointer class? Any other parts of the code that need to be deCOMtaminated?

I meant getting rid of the nsIDateTimeFormat interface and instead having just nsDateTimeFormat (or better yet mozilla::DateTimeFormat). Additionally, I meant letting the methods return whatever type makes sense (e.g. boolean) instead of having to return nsresult.

When the implementation needs to call other stuff that hasn't been deCOMtaminated, it will still need to use nsCOMPtr to hold nsIFoo-typed pointers to other stuff. I guess this mainly applies to the code that asks for the application locale.

Once nsDateTimeFormat/mozilla::DateTimeFormat no longer has an XPCOM interface, there is no longer an *XPCOM-related* need to instantiate the class. Instead, all its methods could become statics as far as deCOMtamination goes.

However, if udat_open is expensive, it might make sense to make the class instantiable such that udat_open() is call upon instantiation and the return value of udat_open() is wrapped in the nsDateTimeFormat/mozilla::DateTimeFormat instance and not taking configuration at the time of the formatting calls. I'm not sure if this performance optimization matters much in practice given the usage patterns. E.g. callers in the certificate manager don't format a huge number of datetimes in one go anyway.

> 2) hsivonen said in comment 3 that we could reduce the number of supported
> format selectors. I believe kDateFormatYearMonth, kDateFormatWeekday,
> kTimeFormatSecondsForce24Hour, and kTimeFormatNoSecondsForce24Hour are never
> used. Should I just remove all code for those selectors, or should I leave
> it in, as people may want to use those formats in the future?

I'd just remove API surface with no current callers.

> 3) The time date type that ICU uses is UDate, which represents the number of
> milliseconds since 1970-jan-01, 00:00 UTC. This is really similar to PRTime
> and time_t, except for PRTime is in microseconds and time_t is seconds. In
> the original code the majority of the logic was in FormatTMTime, as they
> used a tm struct and then if needed, converted it to the appropriate Windows
> or Mac time struct. It seems like there are 3 options for the new code:
> a) Put all of the logic in FormatTime (which takes a time_t), then multiply
> the given time_t by 1000 to get the corresponding UDate value.  
> b) Put all of the logic in FormatPRTime (which takes a PRTime), and then
> divide the given PRTime by 1000 to get the corresponding UDate value (not
> sure if just dividing it would work correctly).
> c) Have a fifth format function (FormatUDate or something) that takes a
> UDate value. Then in FormatTime we could multiply the time_t by 1000 and
> call FormatUDate. In FormatPRTime we could divide the time_t by 1000 and
> call FormatUDate.
> Right now I'm doing option (a). It seems like that's fine because we only
> print at a precision of seconds, so losing any information about
> milliseconds is okay. I'm wondering if doing something like option (c) would
> be better though, as when we call FormatPRTime it calls
> FormatPRExplodedTime, which calls FormatTMTime, which then finally calls
> FormatTime. It seems like a lot of variable conversion that might be
> unnecessary. 
> Also, if were going to get rid of FormatTime and FormatTMTime because they
> don't really have any callers, I guess we would need to do something like
> option (b) because we wouldn't need either of those functions for conversion.
> 
> Let me know, thanks.

I'd go with c) and remove the variants that don't have callers.
Flags: needinfo?(hsivonen)

    
    

    
  

      
      
      
      

        Attached file
          
        DateTimeFormat.cpp (obsolete)
        — 
      

    


  

        Attachment #8799043 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        DateTimeFormat.h (obsolete)
        — 
      

    


  

        Attachment #8799044 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Henri Sivonen (:hsivonen) from comment #36)
> I meant getting rid of the nsIDateTimeFormat interface and instead having
> just nsDateTimeFormat (or better yet mozilla::DateTimeFormat). Additionally,
> I meant letting the methods return whatever type makes sense (e.g. boolean)
> instead of having to return nsresult.

Okay, I see. What should the methods return, by the way? Right now I have them still returning nsresult but changing it to something like bool seems like it would make sense.

> However, if udat_open is expensive, it might make sense to make the class
> instantiable such that udat_open() is call upon instantiation and the return
> value of udat_open() is wrapped in the
> nsDateTimeFormat/mozilla::DateTimeFormat instance and not taking
> configuration at the time of the formatting calls. I'm not sure if this
> performance optimization matters much in practice given the usage patterns.
> E.g. callers in the certificate manager don't format a huge number of
> datetimes in one go anyway.

Okay. I have it static right now, but if we need to change it now or later for better performance I could do that. 

> I'd go with c) and remove the variants that don't have callers.

Okay, I did that. I ended up adding back in the DateTimeFormat function though as I found another call of it here: 
https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsSimplePageSequenceFrame.cpp#322
Since it's also called in the scriptability wrapper I think I'll just leave that function in for now, unless you think we should get rid of it.

Another question I have is about getting the locale. In the original code we would cache the locale in the strings mAppLocale and mLocale. Then we wouldn't have to keep retrieving it if it never changed. Should I do that in the new code, even though it's a static class, and just store it in a static variable?

Please let me know, thanks. I've attached the current version of the code. Sorry for taking so long to reply, by the way.
Flags: needinfo?(hsivonen)

    
    

    
  
(In reply to Gregory Moore from comment #39)
> (In reply to Henri Sivonen (:hsivonen) from comment #36)
> > I meant getting rid of the nsIDateTimeFormat interface and instead having
> > just nsDateTimeFormat (or better yet mozilla::DateTimeFormat). Additionally,
> > I meant letting the methods return whatever type makes sense (e.g. boolean)
> > instead of having to return nsresult.
> 
> Okay, I see. What should the methods return, by the way? Right now I have
> them still returning nsresult but changing it to something like bool seems
> like it would make sense.

If something can fail or succeed and the caller won't care how it failed, bool. If something can't fail, void. If there are multiple failure modes and communicating the failure mode is interesting, nsresult.

> Another question I have is about getting the locale. In the original code we
> would cache the locale in the strings mAppLocale and mLocale. Then we
> wouldn't have to keep retrieving it if it never changed. Should I do that in
> the new code, even though it's a static class, and just store it in a static
> variable?

It's probably a good idea to cache the locale in a static variable, yes.

Sorry about the delay in reply.
Flags: needinfo?(hsivonen)
Priority: -- → P3

    
    

    
  
(In reply to Henri Sivonen (:hsivonen) from comment #40)
> If something can fail or succeed and the caller won't care how it failed,
> bool. If something can't fail, void. If there are multiple failure modes and
> communicating the failure mode is interesting, nsresult.

Okay. I think I will just leave it as an nsresult for now, as there are multiple failure modes (getting the locale can fail, invalid format selectors, or the ICU parts can fail), and because some of the callers are functions that return nsresult anyway. If it needs to be changed later to a bool though it will be really simple.

I think everything is pretty much done though (I changed it so it caches the result now). I also changed all of the callers so they use the new code. Should I split up each change into separate patches, or can I do it all in one big patch?

Also, are there any automated test suites I should run on the build to make sure everything works? Thanks.
Flags: needinfo?(hsivonen)

    
    

    
  
(In reply to Gregory Moore from comment #41)
> I think everything is pretty much done though (I changed it so it caches the
> result now). I also changed all of the callers so they use the new code.
> Should I split up each change into separate patches, or can I do it all in
> one big patch?

Either way works.

> Also, are there any automated test suites I should run on the build to make
> sure everything works? Thanks.

I think mochitest-bc mainly.
Flags: needinfo?(hsivonen)

    
    

    
  


  

        Attachment #8806510 -
        Attachment is obsolete: true

        Attachment #8806511 -
        Attachment is obsolete: true

        Attachment #8810228 -
        Flags: review?(hsivonen)

    
    

    
  
Alright, I ran the tests and they passed. I have one last question about the code.

When getting the locale in the original code, we would use "NSILOCALE_TIME" as the category on Mac and Windows, but "NSILOCALE_TIME##PLATFORM" on the Unix version:
https://dxr.mozilla.org/mozilla-central/source/intl/locale/mac/nsDateTimeFormatMac.cpp#23
https://dxr.mozilla.org/mozilla-central/source/intl/locale/unix/nsDateTimeFormatUnix.cpp#27
In the new code I just used NSILOCALE_TIME for that string, but I've only tested it on Windows so far. I'm wondering if this matters and if I should add some code that chooses a different string based on the operating system. Thanks.

Also, in all of the places where I changed the callers of the functions, do I need to get separate reviews from the owners of each section of code?

    
    

    
  
Comment on attachment 8810228 [details] [diff] [review]
Replaced per-platform variants of nsIDateTimeFormat with a single version implemented on top of ICU.

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

(In reply to Gregory Moore from comment #44)
> Alright, I ran the tests and they passed. I have one last question about the
> code.
> 
> When getting the locale in the original code, we would use "NSILOCALE_TIME"
> as the category on Mac and Windows, but "NSILOCALE_TIME##PLATFORM" on the
> Unix version:
> https://dxr.mozilla.org/mozilla-central/source/intl/locale/mac/
> nsDateTimeFormatMac.cpp#23
> https://dxr.mozilla.org/mozilla-central/source/intl/locale/unix/
> nsDateTimeFormatUnix.cpp#27
> In the new code I just used NSILOCALE_TIME for that string, but I've only
> tested it on Windows so far. I'm wondering if this matters and if I should
> add some code that chooses a different string based on the operating system.
> Thanks.

The variant without ##PLATFORM uses hyphens (as in en-US) and the variant with ##PLATFORM uses underscores (as in en_US). Since ICU accepts locale strings with hyphens, the variant without ##PLATFORM is the correct one to use with ICU on all operating systems.

> Also, in all of the places where I changed the callers of the functions, do
> I need to get separate reviews from the owners of each section of code?

I think the changes are mechanical enough and accommodate changes in the i18n module that a review from an i18n peer should be enough. I'm not one, though, so one you've addressed my review comments, I suggest requesting review from :emk.

On the functionality:

 * The new code seems to follow the structure of the old Windows code in formatting the date and time separately and joining them with a space if there are both. Since udat_format() accepts both a date format and a time format at the same time, is there a reason not to let udat_format handle both in one go (potentially doing something more correct than joining by space for some languages)?

 * If you let udat_format() to format both the date and the time in one go, it seems unnecessary to use intermediate buffers dateBuffer and timeBuffer. Instead, I suggest using stringOut.SetLength(DATETIME_FORMAT_BUFFER_LEN) to stretch the target string, format directly into the buffer returned by stringOut.BeginWriting() and then calling stringOut.SetLength() again with the length udat_format() returns. (With stringOut renamed to aStringOut per below.)

 * Since the code doesn't allocate a larger result buffer anyway when the output is longer than the maximum expected, it seems unnecessary to do a preflight for the length.

 * It seems that the objects allocated by udat_open() should be disposed of using udat_close() instead of plain delete.

There are some formatting issues:

 * There's a bunch of trailing whitespace that should be cleaned up. https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1301640&attachment=8810228 flags them in red.

 * In DateTimeFormat.cpp, please add a line break between the method return type and the method name. (Mozillaism; see: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods )

 * Please have only one space between the type and name in "nsDateFormatSelector dateFormatSelector" (multiple instances).

 * Please prefix argument names with "a" as in "aFoo". (Mozillaism; see: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Prefixes It seems the old code did not conform.)

 * Please name the nsresult-typed to-be-returned local variable "rv" instead of "res". (See the style guide above.)

 * In various places where the methods are called and the argument list wraps to the next line, the wrapped arguments should be aligned to the same column as the first argument.

 * Some more comments inline below.

::: intl/locale/DateTimeFormat.cpp
@@ +8,5 @@
> +#include "nsCOMPtr.h" 
> +#include "nsIServiceManager.h"
> +#include "nsILocaleService.h"
> +#include "unicode/udat.h"
> +

Please wrap the code in this file in
namespace mozilla {
in order to be able to omit the mozilla:: prefix from the identifiers.

@@ +9,5 @@
> +#include "nsIServiceManager.h"
> +#include "nsILocaleService.h"
> +#include "unicode/udat.h"
> +
> +nsCString mozilla::DateTimeFormat::mLocale;

I'm not sure if we're OK with this these days in terms of static initializers/destructors. I'll check.

::: intl/locale/nsScriptableDateFormat.cpp
@@ +102,5 @@
>    timetTime = mktime(&tmTime);
>  
>    if ((time_t)-1 != timetTime) {
> +    rv = mozilla::DateTimeFormat::FormatTime(dateFormatSelector, timeFormatSelector,
> +                                            timetTime, mStringOut);

Please align "timetTime" with "dateFormatSelector".

@@ +113,5 @@
>      if (PR_SUCCESS != PR_ParseTimeString(string, false, &prtime))
>        return NS_ERROR_INVALID_ARG;
>  
> +    rv = mozilla::DateTimeFormat::FormatPRTime(dateFormatSelector, timeFormatSelector,
> +                                              prtime, mStringOut);

Likewise "prtime" here.

::: security/manager/ssl/nsNSSCertHelper.cpp
@@ +1642,2 @@
>                                        kTimeFormatSeconds, &explodedTime,
>                                        tempString);

Please align the arguments on the wrapped lines with the argument on the first line.

@@ +1652,2 @@
>                                        kTimeFormatSeconds, &explodedTimeGMT,
>                                        tempString);

Likewise here.

::: security/manager/ssl/nsNSSCertValidity.cpp
@@ +68,2 @@
>  					     aTimeFormatSelector,
>  					     &explodedTime, aFormattedTimeDate);

Alignment of wrapped arguments.

::: toolkit/components/filepicker/nsFileView.cpp
@@ +729,1 @@
>                                   lastModTime * 1000, temp);

Alignment of wrapped arguments.

        Attachment #8810228 -
        Flags: review?(hsivonen)

    
    

    
  
(In reply to Henri Sivonen (:hsivonen) from comment #45)
> @@ +9,5 @@
> > +#include "nsIServiceManager.h"
> > +#include "nsILocaleService.h"
> > +#include "unicode/udat.h"
> > +
> > +nsCString mozilla::DateTimeFormat::mLocale;
> 
> I'm not sure if we're OK with this these days in terms of static
> initializers/destructors. I'll check.

njn, what's our current stance on nsCString-typed statics? Do we want to make them nsCString* with runtime-allocated pointee instead in order to avoid static initializers/destructors?
Flags: needinfo?(n.nethercote)

    
    

    
  
> > > +nsCString mozilla::DateTimeFormat::mLocale;
> > 
> > I'm not sure if we're OK with this these days in terms of static
> > initializers/destructors. I'll check.
> 
> njn, what's our current stance on nsCString-typed statics? Do we want to
> make them nsCString* with runtime-allocated pointee instead in order to
> avoid static initializers/destructors?

I will defer to froydnj on that...
Flags: needinfo?(n.nethercote) → needinfo?(nfroyd)

    
    

    
  
If there is an obvious point at which the |nsCString*| could be allocated and deleted, then yes, using runtime allocation is preferred.
Flags: needinfo?(nfroyd)

    
    

    
  
Cool, thanks for all of the feedback. I made some of the changes you talked and have some questions about the other ones.

(In reply to Henri Sivonen (:hsivonen) from comment #45)
>  * The new code seems to follow the structure of the old Windows code in
> formatting the date and time separately and joining them with a space if
> there are both. Since udat_format() accepts both a date format and a time
> format at the same time, is there a reason not to let udat_format handle
> both in one go (potentially doing something more correct than joining by
> space for some languages)?

I did have it at one point formatting both the date and time at the same time, with a single UDateFormat object. However, the output was a little different than when I did it separately (like you talk about above). The difference would be something like this:
December 31, 1969 4:00:00 PM
December 31, 1969 at 4:00:00 PM
I thought we wouldn't want to have the "at", so I changed it back to formatting date and time separately. If it would be better to have the extra part in the middle than I change it so it works that way.

>  * If you let udat_format() to format both the date and the time in one go,
> it seems unnecessary to use intermediate buffers dateBuffer and timeBuffer.
> Instead, I suggest using stringOut.SetLength(DATETIME_FORMAT_BUFFER_LEN) to
> stretch the target string, format directly into the buffer returned by
> stringOut.BeginWriting() and then calling stringOut.SetLength() again with
> the length udat_format() returns. (With stringOut renamed to aStringOut per
> below.)

Okay, I will change it so it does it that way if we end up only using one UDateFormat object.

>  * Since the code doesn't allocate a larger result buffer anyway when the
> output is longer than the maximum expected, it seems unnecessary to do a
> preflight for the length.

By preflight do you mean how we call udat_format(), check how long the length will be, and then call udat_format() again if the length is less than DATETIME_FORMAT_BUFFER_LEN? I guess instead we could just pass in DATETIME_FORMAT_BUFFER_LEN as the max buffer length, and then if the resulting length is bigger than that or we get a buffer overflow error we can signal an error using NS_ERROR().

Another question about this is, should we even have the restriction that the result needs to be at most DATETIME_FORMAT_BUFFER_LEN in length? I noticed in the mac version of the original code they don't have this restriction. It seems like something we could potentially take out.

>  * It seems that the objects allocated by udat_open() should be disposed of
> using udat_close() instead of plain delete.

Oh yeah, thanks for catching that. I changed it so it would do that now instead.

(In reply to  Nathan Froyd (:froydnj) from comment #45)
> If there is an obvious point at which the |nsCString*| could be allocated and deleted, 
> then yes, using runtime allocation is preferred.

I think a good place to allocate the string would be during the call to Initialize() (if it's not already allocated). I'm not really sure when we would deallocate it though, as DateTimeFormat is a static class.

I've attached the updated version of the patch with some of the changes made. Thanks.
Flags: needinfo?(hsivonen)

    
    

    
  
(In reply to Gregory Moore from comment #50)
> I did have it at one point formatting both the date and time at the same
> time, with a single UDateFormat object. However, the output was a little
> different than when I did it separately (like you talk about above). The
> difference would be something like this:
> December 31, 1969 4:00:00 PM
> December 31, 1969 at 4:00:00 PM
> I thought we wouldn't want to have the "at", so I changed it back to
> formatting date and time separately. If it would be better to have the extra
> part in the middle than I change it so it works that way.

I didn't look into this too deep, but I'd say that you are using wrong skeleton.

In JS `(new Date()).toLocaleString('en-US');` returns `11/15/2016, 8:09:25 PM` and we are producing it from ICU.

    
    

    
  
Also,

This: `(new Date()).toLocaleString('en-US', {month: 'long', day: 'numeric', year: 'numeric', hour: 'numeric', minute: 'numeric'});`

produces: `November 15, 2016, 11:33 PM`

which is what I believe you're looking for. Not sure how to encode it in C++ ICU, but seems like it should be possible to get this pattern.

    
    

    
  
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #52)
> Also,
> 
> This: `(new Date()).toLocaleString('en-US', {month: 'long', day: 'numeric',
> year: 'numeric', hour: 'numeric', minute: 'numeric'});`
> 
> produces: `November 15, 2016, 11:33 PM`
> 
> which is what I believe you're looking for. Not sure how to encode it in C++
> ICU, but seems like it should be possible to get this pattern.

The JS code path goes through the UDAT_PATTERN route:
https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Intl.cpp#2260

The pattern is generated at
https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Intl.cpp#2188
using 
https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Intl.cpp#2188

The skeleton generation code is at
https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Intl.js#2527

Do we really need to avoid the word "at" so much as to map our option to ICU "skeleton" format code string instead of using the corresponding udat.h options?

I'd just let the word "at" appear.
Flags: needinfo?(hsivonen) → needinfo?(gandalf)

    
    

    
  
(In reply to Nathan Froyd [:froydnj] from comment #48)
> If there is an obvious point at which the |nsCString*| could be allocated
> and deleted, then yes, using runtime allocation is preferred.

Thanks.

(In reply to Gregory Moore from comment #50)
> Cool, thanks for all of the feedback. I made some of the changes you talked
> and have some questions about the other ones.
> 
> (In reply to Henri Sivonen (:hsivonen) from comment #45)
> >  * The new code seems to follow the structure of the old Windows code in
> > formatting the date and time separately and joining them with a space if
> > there are both. Since udat_format() accepts both a date format and a time
> > format at the same time, is there a reason not to let udat_format handle
> > both in one go (potentially doing something more correct than joining by
> > space for some languages)?
> 
> I did have it at one point formatting both the date and time at the same
> time, with a single UDateFormat object. However, the output was a little
> different than when I did it separately (like you talk about above). The
> difference would be something like this:
> December 31, 1969 4:00:00 PM
> December 31, 1969 at 4:00:00 PM
> I thought we wouldn't want to have the "at", so I changed it back to
> formatting date and time separately. If it would be better to have the extra
> part in the middle than I change it so it works that way.

As noted above, I'd let the word "at" appear, but let's head what gandalf thinks.

> By preflight do you mean how we call udat_format(), check how long the
> length will be, and then call udat_format() again if the length is less than
> DATETIME_FORMAT_BUFFER_LEN?

Yes.

> Another question about this is, should we even have the restriction that the
> result needs to be at most DATETIME_FORMAT_BUFFER_LEN in length? I noticed
> in the mac version of the original code they don't have this restriction. It
> seems like something we could potentially take out.

I guess the best option is to first stretch the output string to a reasonable power-of-two length (the underlying allocator has power-of-two buckets, so it makes sense to take the max size a bucket allows), likely 128. Then do the formatting. If the formatting fails, stretch the string to the length required and format again.

> I think a good place to allocate the string would be during the call to
> Initialize() (if it's not already allocated). I'm not really sure when we
> would deallocate it though, as DateTimeFormat is a static class.

The usual way is to introduce a static method DateTimeFormat::Shutdown() and call it from nsLayoutStatics::Shutdown().

    
    

    
  
We should let udat_format() handle the date and time together in a single call, adding whatever separator (space, comma, other text such as "at", ...) it considers appropriate for the locale.

(FWIW, I currently see results such as "16 November 2016 at 12:49:31" in the Page Info window Firefox on OS X, where the platform-specific backend is being used by nsIScriptableDateFormat. Here, the "at" is coming from the Mac's formatting functions. In Nightly, where that use of nsIScriptableDateFormat has been removed in favor of Intl.DateTimeFormat, I see a comma separator instead. But IMO it's no big deal.)

    
    

    
  
Yeah, agree with comment 56. It's not a big enough deal to do anything more complex than use the defaults of ICU.
Let's keep the "at".
Flags: needinfo?(gandalf)

    
    

    
  
Okay great, thanks for all of the feedback from everyone. I made the rest of the changes and uploaded the new version.

The only thing I'm not completely sure about is if we use aStringOut.SetLength(128), does that make the internal string buffer 128 or 129 characters (for the null terminator)? I looked into it and from what I could tell it was 129, and the code works, but I just wanted to make sure.

Also, would I need to run all of the automated tests again because of the new changes (the last time I ran them they took over a day)? If I should that's fine, just checking.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Rebase on top of recent changes (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8812005 -
        Attachment is obsolete: true

        Attachment #8812005 -
        Flags: feedback?(hsivonen)

    
    

    
  
Comment on attachment 8812711 [details] [diff] [review]
Rebase on top of recent changes

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

(In reply to Gregory Moore from comment #59)
> The only thing I'm not completely sure about is if we use
> aStringOut.SetLength(128), does that make the internal string buffer 128 or
> 129 characters (for the null terminator)? I looked into it and from what I
> could tell it was 129, and the code works, but I just wanted to make sure.

Good point! Indeed, the SetLength argument should be 127 to avoid a clown-shoe allocation due to the terminating zero.

> Also, would I need to run all of the automated tests again because of the
> new changes (the last time I ran them they took over a day)? If I should
> that's fine, just checking.

I pushed the patch the try server so that you don't need to run the tests locally:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae8b197f04fc0e92a8de145236de90a7fd6c3000

For reference, the test run for the revision without your patch is:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a385043be65d

I attached the rebased patch.

Looks good with the below nits addressed. Thank you!

::: intl/locale/DateTimeFormat.cpp
@@ +48,5 @@
> +/*static*/ nsresult
> +DateTimeFormat::FormatTime(const nsDateFormatSelector aDateFormatSelector,
> +                                    const nsTimeFormatSelector aTimeFormatSelector,
> +                                    const time_t aTimetTime,
> +                                    nsAString& aStringOut)

It looks like the alignment of the arguments doesn't take into account the removal of the leading "mozilla::" from the line. (This occurs multiple times.)

@@ +60,5 @@
> +                                      const nsTimeFormatSelector aTimeFormatSelector,
> +                                      const PRTime aPrTime,
> +                                      nsAString& aStringOut)
> +{
> +#define DATETIME_FORMAT_INITIAL_LEN 128

As noted, this should, in fact, be 127.

@@ +119,5 @@
> +  UErrorCode status = U_ZERO_ERROR;
> +
> +  UDateFormat* dateTimeFormat = udat_open(timeStyle, dateStyle, mLocale->get(), nullptr, -1, nullptr, -1, &status);
> +
> +  aStringOut.Truncate();

I suggest moving this call to an "else" branch for the immediately following "if", since Truncate() immediately before SetLength() is useless.

@@ +123,5 @@
> +  aStringOut.Truncate();
> +
> +  if (U_SUCCESS(status) && dateTimeFormat) {
> +    aStringOut.SetLength(DATETIME_FORMAT_INITIAL_LEN);
> +    dateTimeLen = udat_format(dateTimeFormat, timeUDate, reinterpret_cast<UChar*>(aStringOut.BeginWriting()), DATETIME_FORMAT_INITIAL_LEN + 1, nullptr, &status);

"+ 1" on this line looks suspicious. Is it documented somewhere that udat_format's notion of length includes the zero terminator? If not, please remove "+ 1".

@@ +128,5 @@
> +
> +    if (status == U_BUFFER_OVERFLOW_ERROR) {
> +      status = U_ZERO_ERROR;
> +      aStringOut.SetLength(dateTimeLen);
> +      udat_format(dateTimeFormat, timeUDate, reinterpret_cast<UChar*>(aStringOut.BeginWriting()), dateTimeLen + 1, nullptr, &status);

Same thing here with "+ 1" with the added twist that if "+ 1" was correct here, udat_format's return value would not count the zero terminator while the argument would, which seems unlikely.

        Attachment #8812711 -
        Flags: feedback+

    
    

    
  
(In reply to Henri Sivonen (:hsivonen) from comment #61)
> I pushed the patch the try server so that you don't need to run the tests
> locally:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ae8b197f04fc0e92a8de145236de90a7fd6c3000
> 
> For reference, the test run for the revision without your patch is:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=a385043be65d
> 
> I attached the rebased patch.

Oh cool, thanks! I noticed on the run with my patch there were some errors that weren't in the other run. For example, the Android sections have errors like:
/home/worker/workspace/build/src/obj-firefox/dist/system_wrappers/unicode/udat.h:3:31: fatal error: unicode/udat.h: No such file or directory
Is this not a problem or should we try to fix this? Just wondering, thanks.

> I suggest moving this call to an "else" branch for the immediately following
> "if", since Truncate() immediately before SetLength() is useless.

I decided to just get rid of it instead, as there didn't seem like a point in clearing the string. If the function fails the caller probably won't care if the string they passed in was cleared or still has the contents it originally had. The Windows and Mac versions of the original code will end up clearing the string if something fails, but the Unix version will just leave it as it is. If there's a reason for doing it I can add it back in though.

> "+ 1" on this line looks suspicious. Is it documented somewhere that
> udat_format's notion of length includes the zero terminator? If not, please
> remove "+ 1".

I guess it doesn't include the null terminator. The example code in the documentation always uses a "+ 1" after the length, but I tested it and it worked fine without the plus one. Thanks for catching that.

I made all of the changes and attached a new patch. I just created an entirely new patch; I'm not sure if I should somehow be using the new rebase attachment you created. Let me know if I should, thanks.
Flags: needinfo?(hsivonen)

    
  

        Attachment #8812711 -
        Attachment is obsolete: true

    
  
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED

    
    

    
  
(In reply to Gregory Moore from comment #63)
> (In reply to Henri Sivonen (:hsivonen) from comment #61)
> > I pushed the patch the try server so that you don't need to run the tests
> > locally:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=ae8b197f04fc0e92a8de145236de90a7fd6c3000
> > 
> > For reference, the test run for the revision without your patch is:
> > https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> > inbound&revision=a385043be65d
> > 
> > I attached the rebased patch.
> 
> Oh cool, thanks! I noticed on the run with my patch there were some errors
> that weren't in the other run. For example, the Android sections have errors
> like:
> /home/worker/workspace/build/src/obj-firefox/dist/system_wrappers/unicode/
> udat.h:3:31: fatal error: unicode/udat.h: No such file or directory
> Is this not a problem or should we try to fix this? Just wondering, thanks.

The Android failures are due to bug 1215247. Landing this patch has to wait for that bug to be fixed.

The unit tests did reveal an actual bug, though:
It's necessary to call SetLength() after the second udat_format call. I added this call in the rebased patch that I attached and re-pushed to try (comment 64).

> > I suggest moving this call to an "else" branch for the immediately following
> > "if", since Truncate() immediately before SetLength() is useless.
> 
> I decided to just get rid of it instead, as there didn't seem like a point
> in clearing the string. If the function fails the caller probably won't care
> if the string they passed in was cleared or still has the contents it
> originally had. The Windows and Mac versions of the original code will end
> up clearing the string if something fails, but the Unix version will just
> leave it as it is. If there's a reason for doing it I can add it back in
> though.

This should be OK, since callers are not supposed to use the string if the return value isn't a success value.

> > "+ 1" on this line looks suspicious. Is it documented somewhere that
> > udat_format's notion of length includes the zero terminator? If not, please
> > remove "+ 1".
> 
> I guess it doesn't include the null terminator. The example code in the
> documentation always uses a "+ 1" after the length, but I tested it and it
> worked fine without the plus one. Thanks for catching that.
> 
> I made all of the changes and attached a new patch.

Thanks.

> I just created an
> entirely new patch; I'm not sure if I should somehow be using the new rebase
> attachment you created. Let me know if I should, thanks.

Please base further work, if needed, on the rebased patch. The patch needs to apply to the tip of the tree to land.

I attached a rebased version of your latest and also added the above-mentioned SetLength fix, added the bug number and a couple of line breaks to the commit message to make our automation like the commit message better.
Assignee: hsivonen → olucafont6
Flags: needinfo?(hsivonen)

    
    

    
  
For clarity, the non-rebase code change I made was:

diff --git a/intl/locale/DateTimeFormat.cpp b/intl/locale/DateTimeFormat.cpp
--- a/intl/locale/DateTimeFormat.cpp
+++ b/intl/locale/DateTimeFormat.cpp
@@ -122,17 +122,18 @@ DateTimeFormat::FormatPRTime(const nsDat
 
   if (U_SUCCESS(status) && dateTimeFormat) {
     aStringOut.SetLength(DATETIME_FORMAT_INITIAL_LEN);
     dateTimeLen = udat_format(dateTimeFormat, timeUDate, reinterpret_cast<UChar
     aStringOut.SetLength(dateTimeLen);
 
     if (status == U_BUFFER_OVERFLOW_ERROR) {
       status = U_ZERO_ERROR;
-      udat_format(dateTimeFormat, timeUDate, reinterpret_cast<UChar*>(aStringOu
+      dateTimeLen = udat_format(dateTimeFormat, timeUDate, reinterpret_cast<UCh
+      aStringOut.SetLength(dateTimeLen);
     }
   }
 
   if (U_FAILURE(status)) {
     rv = NS_ERROR_FAILURE;
   }
 
   if (dateTimeFormat) {

    
    

    
  
(In reply to Henri Sivonen (:hsivonen) from comment #66)
> The unit tests did reveal an actual bug, though:
> It's necessary to call SetLength() after the second udat_format call. I
> added this call in the rebased patch that I attached and re-pushed to try
> (comment 64).

Hmm, that's weird that it needs to call SetLength() again. I noticed that after the first udat_format call we should call SetLength() again, as it's length would be DATETIME_FORMAT_INITIAL_LEN at that point (I made that change in the last patch). I don't really understand why we would call it again after the second udat_format call though (or get a new dateTimeLen), as aStringOut's length will already be dateTimeLen. I guess if it passes the tests that way, it's better though.

> Please base further work, if needed, on the rebased patch. The patch needs
> to apply to the tip of the tree to land.

Okay, I will look into how to do that if I need to make any more changes.

> I attached a rebased version of your latest and also added the
> above-mentioned SetLength fix, added the bug number and a couple of line
> breaks to the commit message to make our automation like the commit message
> better.

Okay cool, thanks for doing that. So should I ask for a review from an i18n peer now? Or should we just wait for the Android bug to be fixed first? Thanks.
Flags: needinfo?(hsivonen)

    
    

    
  
(In reply to Gregory Moore from comment #68)
> (In reply to Henri Sivonen (:hsivonen) from comment #66)
> > The unit tests did reveal an actual bug, though:
> > It's necessary to call SetLength() after the second udat_format call. I
> > added this call in the rebased patch that I attached and re-pushed to try
> > (comment 64).
> 
> Hmm, that's weird that it needs to call SetLength() again. I noticed that
> after the first udat_format call we should call SetLength() again, as it's
> length would be DATETIME_FORMAT_INITIAL_LEN at that point (I made that
> change in the last patch). I don't really understand why we would call it
> again after the second udat_format call though (or get a new dateTimeLen),
> as aStringOut's length will already be dateTimeLen. I guess if it passes the
> tests that way, it's better though.

Good point. Yet, the previous patch showed this failure:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae8b197f04fc0e92a8de145236de90a7fd6c3000&selectedJob=31570539

Now that I think about it, the output is so short that it shouldn't have reached the second udat_format call anyway.

Sorry about adding a "fix" that wasn't. Did something else change that could explain that failure going away in the most recent run?

> Okay cool, thanks for doing that. So should I ask for a review from an i18n
> peer now? Or should we just wait for the Android bug to be fixed first?

I suggest requesting review now in order to have the patch ready to land when we get ICU on Android.
Flags: needinfo?(hsivonen)

    
    

    
  
(In reply to Henri Sivonen (:hsivonen) from comment #69)
> Sorry about adding a "fix" that wasn't. Did something else change that could
> explain that failure going away in the most recent run?

Yeah, I did change something else that I think would have fixed that problem. I forgot to mention it when I uploaded my last patch (I sort of mentioned it in comment 68), but I noticed a potential problem so I moved one line of code to fix it. Ignoring the other changes I made, it basically looked like this:

    aStringOut.SetLength(DATETIME_FORMAT_INITIAL_LEN);
    dateTimeLen = udat_format(dateTimeFormat, timeUDate, reinterpret_cast<UChar*>
++  aStringOut.SetLength(dateTimeLen);
    if (status == U_BUFFER_OVERFLOW_ERROR) {
--    aStringOut.SetLength(dateTimeLen);
      status = U_ZERO_ERROR;
      udat_format(dateTimeFormat, timeUDate, reinterpret_cast<UChar*>
    }

Before, after we resized the aStringOut to DATETIME_FORMAT_INITIAL_LEN, we would only change it's length if a buffer overflow occurred. I changed it so it would update the length in either case (otherwise the string would think it had DATETIME_FORMAT_INITIAL_LEN valid characters, when it really had less than that).

I'm pretty sure that's why it works now. We can probably get rid of the change you talked about in comment 67 (if it still passes the tests, at least). I can try making the changes to your rebased version, or if you just want to make the changes that would probably be simpler. Let me know, thanks.

> I suggest requesting review now in order to have the patch ready to land
> when we get ICU on Android.

Alright cool, I will do that after this last problem is fixed.

    
    

    
  
(In reply to Gregory Moore from comment #70)
> I'm pretty sure that's why it works now. We can probably get rid of the
> change you talked about in comment 67 (if it still passes the tests, at
> least). I can try making the changes to your rebased version, or if you just
> want to make the changes that would probably be simpler. Let me know, thanks.

That would be good. Thanks!

    
    

    
  


  

        Attachment #8812980 -
        Attachment is obsolete: true

        Attachment #8814756 -
        Flags: review?(VYV03354)

    
    

    
  
Okay cool, I changed it back to how it was originally and rebased it again. If you could run it through the try server one more time (just to make sure it still works), that would be great. Thanks.

    
    

    
  
Comment on attachment 8814756 [details] [diff] [review]
Rebased again, removed SetLength call added in last patch.

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

Could you leave the non-ICU Unix backend to make the patch landable without waiting for bug 1215247? I don't think it will actually be fixed. I would not like to rebase the patch forever and block Intl API bugs on desktop.

Probably you want to rename the current DateTimeFormat.cpp to DateTimeFormatICU.cpp and deCOMtaminate nsDateTimeFormatUnix.cpp.

You can test the ICU support in moz.build such as:

if CONFIG['ENABLE_INTL_API']:
    UNIFIED_SOURCES += [
        'DateTimeFormatICU.cpp',
    ]
else:
    UNIFIED_SOURCES += [
        'DateTimeFormatUnix.cpp',
    ]

::: intl/locale/DateTimeFormat.h
@@ +3,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef DateTimeFormat_h__

Please do not use double underscore in new files. It violates the C++ standard. Also it should contain namespace prefix to avoid collision. That is, it should look like:

  #ifndef mozilla_DateTimeFormat_h

        Attachment #8814756 -
        Flags: review?(VYV03354)

        Attachment #8813123 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Masatoshi Kimura [:emk] from comment #74)
> Could you leave the non-ICU Unix backend to make the patch landable without
> waiting for bug 1215247? I don't think it will actually be fixed. I would
> not like to rebase the patch forever and block Intl API bugs on desktop.
> 
> Probably you want to rename the current DateTimeFormat.cpp to
> DateTimeFormatICU.cpp and deCOMtaminate nsDateTimeFormatUnix.cpp.
> 
> You can test the ICU support in moz.build such as:
> 
> if CONFIG['ENABLE_INTL_API']:
>     UNIFIED_SOURCES += [
>         'DateTimeFormatICU.cpp',
>     ]
> else:
>     UNIFIED_SOURCES += [
>         'DateTimeFormatUnix.cpp',
>     ]

This sounds like a good idea. If that bug ever gets fixed we can get rid of the Unix version and just use the ICU version.

I started creating a deCOMtaminated version nsDateTimeFormatUnix.cpp and thought of some problems/questions about this:

1) Earlier in this thread/topic hsivonen was saying that we should stop calling setlocale() in the Unix version, if we weren't going to replace it with ICU (comment 16, comment 19). So should I just take out all calls to setlocale() in the new version?

2) If we want to have one interface (DateTimeFormat.h) but two implementations (DateTimeFormatICU.cpp, DateTimeFormatUnix.cpp), than the member variables will have to be the same. Currently the ICU code only has member variable (nsCString* mLocale), but the Unix version might need all these:

static nsCString* mCharset; // in order to convert API result to unicode
static nsCString* mPlatformLocale; // for setlocale
static bool mLocalePreferred24hour; // true if 24 hour format is preferred by current locale                       
static bool mLocaleAMPMfirst; // true if AM/PM string is preferred before the time                             
static nsCOMPtr <nsIUnicodeDecoder> mDecoder;

If we don't need to call setlocale than I guess we can get rid of mPlatformLocale. Not sure if we can get rid of any of the other ones though (without having to calculate them at each call).

So I'm wondering if we can do a #ifdef-#elif-#endif inside of DateTimeFormat.h, similar to:
https://dxr.mozilla.org/mozilla-central/source/intl/locale/nsIDateTimeFormat.cpp
Then depending on if the operating system supports ICU, we could define different private member variables within the #ifdef (not sure if you can actually do this). In the build file we would do:
if CONFIG['ENABLE_INTL_API']:
Is there a way to check the same condition with an #ifdef? 

Also, I don't have Unix/Linux installed on any of my computers. Would I need to install that and set up all the build stuff to test the Unix version, or is there a simpler way? Thanks.
Flags: needinfo?(hsivonen)

    
    

    
  
(In reply to Gregory Moore from comment #75)
> (In reply to Masatoshi Kimura [:emk] from comment #74)
> > Could you leave the non-ICU Unix backend to make the patch landable without
> > waiting for bug 1215247? I don't think it will actually be fixed. I would
> > not like to rebase the patch forever and block Intl API bugs on desktop.
> > 
> > Probably you want to rename the current DateTimeFormat.cpp to
> > DateTimeFormatICU.cpp and deCOMtaminate nsDateTimeFormatUnix.cpp.
> > 
> > You can test the ICU support in moz.build such as:
> > 
> > if CONFIG['ENABLE_INTL_API']:
> >     UNIFIED_SOURCES += [
> >         'DateTimeFormatICU.cpp',
> >     ]
> > else:
> >     UNIFIED_SOURCES += [
> >         'DateTimeFormatUnix.cpp',
> >     ]
> 
> This sounds like a good idea. If that bug ever gets fixed we can get rid of
> the Unix version and just use the ICU version.
> 
> I started creating a deCOMtaminated version nsDateTimeFormatUnix.cpp and
> thought of some problems/questions about this:
> 
> 1) Earlier in this thread/topic hsivonen was saying that we should stop
> calling setlocale() in the Unix version, if we weren't going to replace it
> with ICU (comment 16, comment 19). So should I just take out all calls to
> setlocale() in the new version?

Yes.

> 2) If we want to have one interface (DateTimeFormat.h) but two
> implementations (DateTimeFormatICU.cpp, DateTimeFormatUnix.cpp), than the
> member variables will have to be the same. Currently the ICU code only has
> member variable (nsCString* mLocale), but the Unix version might need all
> these:
> 
> static nsCString* mCharset; // in order to convert API result to unicode
> static nsCString* mPlatformLocale; // for setlocale
> static bool mLocalePreferred24hour; // true if 24 hour format is preferred
> by current locale                       
> static bool mLocaleAMPMfirst; // true if AM/PM string is preferred before
> the time                             
> static nsCOMPtr <nsIUnicodeDecoder> mDecoder;
> 
> If we don't need to call setlocale than I guess we can get rid of
> mPlatformLocale. Not sure if we can get rid of any of the other ones though
> (without having to calculate them at each call).
> 
> So I'm wondering if we can do a #ifdef-#elif-#endif inside of
> DateTimeFormat.h, similar to:
> https://dxr.mozilla.org/mozilla-central/source/intl/locale/nsIDateTimeFormat.
> cpp
> Then depending on if the operating system supports ICU, we could define
> different private member variables within the #ifdef (not sure if you can
> actually do this). In the build file we would do:
> if CONFIG['ENABLE_INTL_API']:
> Is there a way to check the same condition with an #ifdef? 

#ifdef ENABLE_INTL_API should work.

> Also, I don't have Unix/Linux installed on any of my computers. Would I need
> to install that and set up all the build stuff to test the Unix version, or
> is there a simpler way? Thanks.

I don't think there's anything simpler than installing Linux on VirtualBox and setting up the build environment there. :-(

I suggest asking for the estimated timeline over at the Android bug (bug 1215247) before spending too much time working around it. The Android bug was stuck in a bad state for a very long time, but bug 1215247 comment 84 and bug 1215247 comment 86 suggest it's finally on track to being fixed (which is why I filed this bug).
Flags: needinfo?(hsivonen)

    
    

    
  
(In reply to Gregory Moore from comment #75)
> 1) Earlier in this thread/topic hsivonen was saying that we should stop
> calling setlocale() in the Unix version, if we weren't going to replace it
> with ICU (comment 16, comment 19). So should I just take out all calls to
> setlocale() in the new version?

You can replace setlocale/strftime with newlocale/strftime_l/freelocale. You don't have to (and should not) create and destroy a locale_t instance every time you call strftime_l. Rather, the locale_t instance should be created only when mPlatformLocale is changed.

> 2) If we want to have one interface (DateTimeFormat.h) but two
> implementations (DateTimeFormatICU.cpp, DateTimeFormatUnix.cpp), than the
> member variables will have to be the same. Currently the ICU code only has
> member variable (nsCString* mLocale), but the Unix version might need all
> these:
> 
> static nsCString* mCharset; // in order to convert API result to unicode
> static nsCString* mPlatformLocale; // for setlocale
> static bool mLocalePreferred24hour; // true if 24 hour format is preferred
> by current locale                       
> static bool mLocaleAMPMfirst; // true if AM/PM string is preferred before
> the time                             
> static nsCOMPtr <nsIUnicodeDecoder> mDecoder;
> 
> If we don't need to call setlocale than I guess we can get rid of
> mPlatformLocale. Not sure if we can get rid of any of the other ones though
> (without having to calculate them at each call).
> 
> So I'm wondering if we can do a #ifdef-#elif-#endif inside of
> DateTimeFormat.h, similar to:
> https://dxr.mozilla.org/mozilla-central/source/intl/locale/nsIDateTimeFormat.
> cpp
> Then depending on if the operating system supports ICU, we could define
> different private member variables within the #ifdef (not sure if you can
> actually do this). In the build file we would do:
> if CONFIG['ENABLE_INTL_API']:
> Is there a way to check the same condition with an #ifdef? 

#ifdef ENABLE_INTL_API as Henri said.

> Also, I don't have Unix/Linux installed on any of my computers. Would I need
> to install that and set up all the build stuff to test the Unix version, or
> is there a simpler way? Thanks.

You can add "ac_add_options --without-intl-api" to your .mozconfig to temporary disable ENABLE_INTL_API.
nsIDateTimeUnix.cpp looks enough portable to compile with MSVC. Unfortunately newlocale/strftime_l/freelocale are not so portable. But you can add a shim such as:
#ifdef _MSC_VER
using locale_t = _locale_t;
static inline locale_t
newlocale(int category, const char* locale, locale_t base)
{
  return _create_locale(category, locale);
}
static const auto freelocale = _free_locale;
static const auto strftime_l = _strftime_l;
#endif

I'll kick a try run to verify that it works on the real Android once you attached a new patch.

    
    

    
  
(In reply to Henri Sivonen (:hsivonen) from comment #76)
> I suggest asking for the estimated timeline over at the Android bug (bug
> 1215247) before spending too much time working around it. The Android bug
> was stuck in a bad state for a very long time, but bug 1215247 comment 84
> and bug 1215247 comment 86 suggest it's finally on track to being fixed
> (which is why I filed this bug).

Those comments are 3 months ago. I couldn't find bug 1215247 in <https://github.com/mozilla-l10n/roadmap/projects/1> and <https://github.com/mozilla-l10n/roadmap/projects/2>. I don't believe they are willing to fix bug 1215247 unless it is atually fixed.

    
    

    
  
(In reply to Masatoshi Kimura [:emk] from comment #77)
> You don't have to (and should not) create and destroy a locale_t instance every
> time you call strftime_l. Rather, the locale_t instance should be created
> only when mPlatformLocale is changed.

I changed the mind. It would be safer to create/use/destroy every time from the thread-safety perspective.

    
    

    
  
Hi, sorry for taking a while to respond, and thanks for all of the information. I asked in bug 1215247 for an estimated timeline a few days ago, but no one has responded yet.

I did the things you guys talked about and created a version of the changes with a non-ICU Unix backend. I've only tried it on Windows (using the shims and a couple other workarounds), but it works on there (though its probably not finished). Let me know if we should wait for the Android bug to get fixed, or try to finish this version. Either way is fine, thanks.
Flags: needinfo?(hsivonen)

    
    

    
  
I strongly recommend to go ahead now, even thinking about that Mozilla employees were busy this week due to All Hands.

    
    

    
  
Based on some informal queries I've made this week, I believe we're still on track to getting ICU on Android, but with enough dependencies that it's probably worthwhile to work around Android not having ICU to get this patch landed.
Flags: needinfo?(hsivonen)

    
    

    
  
Comment on attachment 8817747 [details] [diff] [review]
Initial version with non-ICU Unix backend left in.

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

Okay cool, sounds good. Besides the problems I asked about above and some formatting stuff, I can't find any other problems yet. Let me know if there's other stuff I need to change. Thanks.

::: intl/locale/DateTimeFormatUnix.cpp
@@ +12,5 @@
> +#include "nsIPlatformCharset.h" 
> +#include "nsPosixLocale.h" 
> +#include "nsCRT.h"
> +#include "nsReadableUtils.h" 
> +#include "nsUnicharUtils.h" 

I'm not sure if we need some of these includes anymore. I tried removing the includes for nsIServiceManager.h, nsCRT.h, nsReadableUtils.h, and nsUnicharUtils.h, and it still compiled (at least on Windows). It seems like we're not using the functions/classes in nsUnicharUtils.h anymore, but I'm not sure if we should remove the other files or not.

@@ +110,5 @@
> +  locale_t loc = newlocale(LC_TIME, mPlatformLocale->get(), (locale_t)0);
> +
> +  if (loc == (locale_t)0)
> +    return; 
> +

When we call newlocale() here (and on line 200), if it fails it will return (locale_t)0. Should we check this and then react accordingly, or should we just assume the call will succeed?

@@ +196,5 @@
> +    }
> +  }
> +
> +  // generate data/time string
> +  locale_t loc = newlocale(LC_TIME, mPlatformLocale->get(), (locale_t)0);

After this line we could do:

if (loc == (locale_t)0)
  return NS_ERROR_FAILURE;

But on line 100 in LocalePreferred24hour(), the function has a void return value, so I'm not really sure what to do there. Should I change it so that LocalePreferred24hour() returns a bool, and then have Initialize() check the return value and return NS_ERROR_FAILURE if it fails? Currently FormatTMTime() just ignores the return value of Initialize(), but we could change it checks it.

On the other hand, he only thing that LocalePreferred24hour() does is set the values of mLocalePreferred24hour and mLocaleAMPMfirst, so it probably won't matter if we don't know if failed. Also, if the call to newlocale() fails within LocalePreferred24hour() it will likely fail again within FormatTMTime(). So we probably won't even ending up formatting the string or using the values in mLocalePreferred24hour and mLocaleAMPMfirst in that case.

        Attachment #8817747 -
        Flags: feedback?(hsivonen)

    
    

    
  
Comment on attachment 8817747 [details] [diff] [review]
Initial version with non-ICU Unix backend left in.

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

::: intl/locale/DateTimeFormatUnix.cpp
@@ +12,5 @@
> +#include "nsIPlatformCharset.h" 
> +#include "nsPosixLocale.h" 
> +#include "nsCRT.h"
> +#include "nsReadableUtils.h" 
> +#include "nsUnicharUtils.h" 

Please use SOURCE (instead of UNIFIED_SOURCE) in moz.build to verify whether some included files are needed. Don't forget changing back to UNIFIED_SOURCE after the verification.

@@ +110,5 @@
> +  locale_t loc = newlocale(LC_TIME, mPlatformLocale->get(), (locale_t)0);
> +
> +  if (loc == (locale_t)0)
> +    return; 
> +

Before I answer the question, could you use "hg mv" to rename nsDateTimeFormatUnix.cpp to DateTimeFormatUnix.cpp to find changes easier and preserve the history?

    
    

    
  


  

        Attachment #8817747 -
        Attachment is obsolete: true

        Attachment #8817747 -
        Flags: feedback?(hsivonen)

    
  

        Attachment #8818121 -
        Attachment description: bug1301640_nsidatetimeformatonicu.patch → Renamed nsDateTimeFormatUnix.cpp to DateTimeFormatUnix.cpp, removed unnecessary #includes.

    
    

    
  
(In reply to Masatoshi Kimura [:emk] from comment #85)
> Please use SOURCE (instead of UNIFIED_SOURCE) in moz.build to verify whether
> some included files are needed. Don't forget changing back to UNIFIED_SOURCE
> after the verification.

Okay, I changed it so we would only add DateTimeFormat*.cpp to SOURCES in moz.build and it still built fine. I'll just leave them out for now; if we need to add them back in later we can though.

> Before I answer the question, could you use "hg mv" to rename
> nsDateTimeFormatUnix.cpp to DateTimeFormatUnix.cpp to find changes easier
> and preserve the history?

Sure, I attached an updated version. Thanks.

    
    

    
  
(In reply to Gregory Moore from comment #84)
> @@ +110,5 @@
> > +  locale_t loc = newlocale(LC_TIME, mPlatformLocale->get(), (locale_t)0);
> > +
> > +  if (loc == (locale_t)0)
> > +    return; 
> > +
> 
> When we call newlocale() here (and on line 200), if it fails it will return
> (locale_t)0. Should we check this and then react accordingly, or should we
> just assume the call will succeed?
> 
> @@ +196,5 @@
> > +    }
> > +  }
> > +
> > +  // generate data/time string
> > +  locale_t loc = newlocale(LC_TIME, mPlatformLocale->get(), (locale_t)0);
> 
> After this line we could do:
> 
> if (loc == (locale_t)0)
>   return NS_ERROR_FAILURE;
> 
> But on line 100 in LocalePreferred24hour(), the function has a void return
> value, so I'm not really sure what to do there. Should I change it so that
> LocalePreferred24hour() returns a bool, and then have Initialize() check the
> return value and return NS_ERROR_FAILURE if it fails? Currently
> FormatTMTime() just ignores the return value of Initialize(), but we could
> change it checks it.
> 
> On the other hand, he only thing that LocalePreferred24hour() does is set
> the values of mLocalePreferred24hour and mLocaleAMPMfirst, so it probably
> won't matter if we don't know if failed. Also, if the call to newlocale()
> fails within LocalePreferred24hour() it will likely fail again within
> FormatTMTime(). So we probably won't even ending up formatting the string or
> using the values in mLocalePreferred24hour and mLocaleAMPMfirst in that case.

It would be better to make the best effort to return something because many callers are not ready to handle failures. Maybe fallback to the global locale version of strftime?

But... I noticed that the Android implementation of strftime_l ignores the locale parameter and just calls strftime. Given that DateTimeFormatUnix is left only for Android, it would be the simplest to just remove all setlocale calls and forget about newlocale/strftime_l/freelocale. Sorry for wasting your time.

    
    

    
  


  

        Attachment #8818121 -
        Attachment is obsolete: true

        Attachment #8818378 -
        Flags: feedback?(VYV03354)

    
    

    
  
(In reply to Masatoshi Kimura [:emk] from comment #88)
> But... I noticed that the Android implementation of strftime_l ignores the
> locale parameter and just calls strftime. Given that DateTimeFormatUnix is
> left only for Android, it would be the simplest to just remove all setlocale
> calls and forget about newlocale/strftime_l/freelocale. Sorry for wasting
> your time.

Oh, that's too bad. No problem though; I had to change other stuff, so it wasn't really a waste of time.

I updated it so it just uses strftime() and doesn't call setlocale(). Let me know if there are other things we should change, thanks.
Flags: needinfo?(VYV03354)

    
    

    
  
Comment on attachment 8818378 [details] [diff] [review]
Removed calls to newlocale/strftime_l/freelocale

Thank you. This patch looks good at first glance. I kicked a try run to see what's going on:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a1924e14cfeaafc0c21a93529c262b5019798bd
Flags: needinfo?(VYV03354)

    
    

    
  
Comment on attachment 8818378 [details] [diff] [review]
Removed calls to newlocale/strftime_l/freelocale

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

Try looks good.

::: intl/locale/DateTimeFormat.h
@@ +56,5 @@
> +                               nsAString& stringOut);
> +
> +  static void LocalePreferred24hour();
> +
> +  //static nsCString* mPlatformLocale;

Is this still necessary?

        Attachment #8818378 -
        Flags: feedback?(VYV03354) → feedback+

    
    

    
  
Comment on attachment 8818378 [details] [diff] [review]
Removed calls to newlocale/strftime_l/freelocale

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

(In reply to Masatoshi Kimura [:emk] from comment #92)
> Is this still necessary?

No, it's not. I accidentally left that in. Thanks for catching that. 

Also, here it says that static members should have a prefix of "s":
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Prefixes
So should we change the static member variables to have the prefix "s"? Or "ms" or "sm" or something else? I also found some other formatting problems that I fixed.

::: intl/locale/DateTimeFormatUnix.cpp
@@ +239,5 @@
>  
> +/*static*/ void
> +DateTimeFormat::Shutdown()
> +{
> +}

Because we no longer need the static member variable mPlatformLocale, this function doesn't need to do anything in DateTimeFormatUnix.cpp (but we still need it for DateTimeFormatICU.cpp). Should I write it on one line or just leave it like this, or is there cleaner way to do this? Thanks.

    
  
Flags: needinfo?(VYV03354)

    
    

    
  
Personally I don't care those style nits. But please avoid "sm" or "ms". I have never seen such prefixes in our source tree.
Flags: needinfo?(VYV03354)

    
    

    
  


  

        Attachment #8818378 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Masatoshi Kimura [:emk] from comment #94)
> Personally I don't care those style nits. But please avoid "sm" or "ms". I
> have never seen such prefixes in our source tree.

Okay, thanks. I will just leave the prefixes as m then. I've attached an updated patch.

    
    

    
  
Comment on attachment 8820509 [details] [diff] [review]
Fixed formatting errors, rebased again

Do you consider further improvements? Or do you want me to review this version? Please set the review flag if the latter.
Flags: needinfo?(olucafont6)

    
    

    
  
I'm not sure doc is really needed. nsIDateTimeFormat is not scriptable and we dropped support for binary addons, so no one can call this interface from external.

    
  

        Attachment #8820509 -
        Flags: review?(VYV03354)

    
    

    
  
(In reply to Masatoshi Kimura [:emk] from comment #97)
> Do you consider further improvements? Or do you want me to review this
> version? Please set the review flag if the latter.

The patch mostly just fixes some formatting problems (and I also rebased it again). I think the patch is pretty much done, but if you could take one last look at it and run it through the try server, that would be great. Thanks.
Flags: needinfo?(olucafont6)

    
    

    
  
Comment on attachment 8820509 [details] [diff] [review]
Fixed formatting errors, rebased again

Look good to me and green on try.

Thank you for contributing Mozilla!

        Attachment #8820509 -
        Flags: review?(VYV03354) → review+

    
    

    
  
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e747433d1c9
Replaced per-platform variants of nsIDateTimeFormat with single, ICU-backed version that has static c++ callers. r=emk

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/7e747433d1c9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago

          status-firefox53:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

    
    

    
  
I would like to complain about this:
1) This got landed busting Thunderbird big time with no heads-up.
2) You removed the locale parameter from FormatPRTime() which we were actually using.

Worst of all, this actually doesn't appear to work correctly.

At least on Windows, the date/time format does not follow any more what the user configured.

I have an English version of Windows 7 with *Australian* date/time format. So I used to get the following in the Thunderbird thread pane:

Before this patch:
Received: 23/12/2016 22:42
Correct as configured.

Now:
Received: 23/12/2016, 10:42 pm
Looks like the time format that applies to the application version of en-US.

Surely FF also displays times in some spots, so will all these now disobey the user's chosen format?
Flags: needinfo?(olucafont6)
Flags: needinfo?(hsivonen)
Flags: needinfo?(VYV03354)
Depends on: 1325745
Blocks: 1325751

    
    

    
  
I am really confused by the discussion in this bug. Comment #7 already warns that ICU doesn't give 12/24h format or honor any OS specific configuration, similar in comment #15. In comment #18 Henri confirmed that you can configure Windows to show YYYY-MM-DD. And then the discussion drifts away, from about comment #35 you start discussing the code and in the end something that *loses existing functionality* is landed.

Can someone please point out the comment where the firm decision is made and agreed that this should go ahead ignoring all user configuration.

It's hard to real 100 comments and find the important bits, I did however see comment #19 where option 4) suggested not to use ICU but simply to lose the 'locale' parameter of FormatPRTime() and friends. That option makes a whole lot of sense to me.

Personally, I'd like to see this backed out until an integrated solution is presented.

    
    

    
  
Which locale is this using? The system locale or the application locale?

I changed the system locale on Windows 7 from English/US to English/AU to English/GB to German/DE and I still get the very same time display with "am" and "pm". So it must be using the application locale. That's really no good since may people use the en-US version of FF and have it use the system locale or user's configuration. That would be true for all users in India, for example. en-US is used 100% in India, however, India being a Commonwealth country, they don't use the US formats.

Check:
http://bsmedberg.github.io/telemetry-dashboard/new-pipeline/active-weekly.html
to see that what I said about India is correct.
Depends on: 1325792

    
    

    
  
I backed out this change:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6882a49e907bd01ab7b782a0985614f4565e02e

Probably holiday season is not the good timing to make such large changes.
Flags: needinfo?(VYV03354)

    
    

    
  
Yeah, I'm pushing on ICU and ECMA402 to come up with a plan to tackle the OS env. preferences use, but it'll take some time.

We should come up with chrome-only way to tackle that for now.

I see three possible approaches:

1) We can write an external API and use it together with Intl API
2) We can write mozIntl extension (mozIntl.DateTimeFormat) which will be a wrapper that additionally look into OS prefs
3) We can try to hack chrome-only addition to Intl API.

I would suggest (2) for now.

The open challenge still is how to get it to take OS preferences when they exist, but not override CLDR defaults when there are no OS preferences set.

The limitation is that if we'll not go with (3) and not extend our Intl API to take `pattern` option, there's no way for us to alter the skeleton.

Also, I believe it would be worth, if we'll go for (2) to introduce long/medium/short/narrow date/time/datetime style in mozIntl.DateTimeFormat, set some sane default options for them and allow localizers to override them.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #108)
> The open challenge still is how to get it to take OS preferences when they
> exist, but not override CLDR defaults when there are no OS preferences set.

On a philosophical level I don't see how that would be preferable. The OS knows best what the user wants, so if CLDR overrides that and differs, CLDR is wrong. I.e. CLDR defaults should only be used if the OS can't give a statement for whatever reason.

    
    

    
  
Comment on attachment 8820509 [details] [diff] [review]
Fixed formatting errors, rebased again

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

Forgive the drive by review...

::: security/manager/ssl/nsNSSCertValidity.cpp
@@ +65,4 @@
>    PRExplodedTime explodedTime;
>    PR_ExplodeTime(const_cast<PRTime&>(aTimeDate), aParamFn, &explodedTime);
> +  return mozilla::DateTimeFormat::FormatPRExplodedTime(kDateFormatLong,
> +					                                             aTimeFormatSelector,

The indentation here is off.

    
    

    
  
(In reply to Magnus Melin from comment #109)
> On a philosophical level I don't see how that would be preferable.

I have noticed your struggle. You responded in all related bugs with it.

> The OS knows best what the user wants

That's your opinion. If you want to participate in the conversation, I'd appreciate if you recognized that and notice that other people have a different one.
It would also help if you assumed that their opinion is based on knowledge and experience. That makes it easier to talk.

    
    

    
  
(In reply to Masatoshi Kimura [:emk] from comment #107)
> I backed out this change:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a6882a49e907bd01ab7b782a0985614f4565e02e
> Probably holiday season is not the good timing to make such large changes.
Indeed. Thanks for the backout. The comment reads:
Backed out changeset 7e747433d1c9 (bug 1301640) for causing bug 1325751

I'm still trying to understand the following:

Was this really backed out for causing bug 1325751, that is, not honouring the formats defined in the OS (at least for Windows), which was already known when landing it (see various comment at the beginning of this bug).

Or was it backed out because, at least in my testing, it didn't achieve what it set out to do, that is use the formats according to the set browser locale in general.useragent.locale? I noticed that I could set this preference to en-AU, en-GB, de-DE, it didn't matter, I still got the en-US date/time format. Over in bug 1325751 Zibi suggested that ICU uses CLDR and the formats should have followed the browser's locale setting.

Personally I don't really care how the date/time format is configured, via the OS or the browser locale, but with this patch I got straight en-US formats and there didn't appear a way to change this at all.

    
    

    
  
:emk - should we set bug 1308329 as blocking this one?

    
    

    
  
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #113)
> :emk - should we set bug 1308329 as blocking this one?

IMO we should do it to mitigate the impact of some locales.
Depends on: 1308329

    
    

    
  
And we should send "Intent to implement" to dev.platform before we restart work on this bug.

    
    

    
  
Hm, our current nsLocaleService implementation will take the app locale from the operating system:
https://dxr.mozilla.org/mozilla-central/rev/5ea0c495d3b2318287bffe1121e0e33d74427143/intl/locale/nsLocaleService.cpp#94

And DateTimeFormatICU is supposed to use the app locale:
https://hg.mozilla.org/mozilla-central/diff/7e747433d1c9/intl/locale/DateTimeFormatICU.cpp#l1.37

We should investigate why this patch no longer respects the operating system locale.

    
    

    
  
Yeah, the patch should make us follow the browser locale taken from `localeService->GetApplicationLocale` which should be the same as `general.useragent.locale`.

The goal at the moment should be for all Intl and L10n services to use this locale across the UI and web-content.

    
    

    
  
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #117)
> Yeah, the patch should make us follow the browser locale taken from
> `localeService->GetApplicationLocale` which should be the same as
> `general.useragent.locale`.

Even if it should be, the current implementation is not. So this patch should have not caused bug  	1325751. That is, the patch is plain simple broken. We should fix the bug regardless how localeService->GetApplicationLocale should behave.

    
    

    
  
(In reply to Masatoshi Kimura [:emk] from comment #118)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #117)
> > Yeah, the patch should make us follow the browser locale taken from
> > `localeService->GetApplicationLocale` which should be the same as
> > `general.useragent.locale`.
> 
> Even if it should be, the current implementation is not.

To be crystal clear, the current localeService->GetApplicationLocale() is NOT the same as "general.useragent.locale" even if it should be.

    
    

    
  
> Even if it should be, the current implementation is not.

Hmmm, so, the current implementation will render dates based on my OS locale irrelevant of what the browser locales is?

I'm wondering if there's a pile of bugs from people who switched their browser UI locale and the date/time didn't follow... :)

Also, it seems then, that we get different app locale from JS and C++, which should give us another pile of bugs.

I believe my team's (l10n drivers) recommendation is to unify everything under browser UI locale for both JS and C++.

NI'ing :pike to double check.
Flags: needinfo?(l10n)

    
    

    
  
Also, you should not read the "general.useragent.locale" pref directly.

You should use Locale.getLocale() from Locale.jsm to get the browser UI locale in JS. Again, this method is not the same as "general.useragent.locale". This method will also consider the "intl.locale.matchOS" pref. And the actual selected language pack will match the Locale.getLocale() result.

It oversimplifies the situation to treat the browser UI locale is nothing but "general.useragent.locale".

    
    

    
  
general.useragent.locale has little to do with the UI locale.
Also, navigator.languages has little to do with the UI locale.

The recommended way to get the UI locale today is:

Cc['@mozilla.org/chrome/chrome-registry;1'].getService(Ci.nsIXULChromeRegistry).getSelectedLocale('global')

Sadly, https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Locale.jsm#20 uses brittle code paths, too. In particular, you can enter random junk as your selected locale in a non-matchOS single-locale build, and it will return that.
Flags: needinfo?(l10n)

    
    

    
  
That's... surprising.

I filed bug 1325870 to iron out a vision for the browser/webcontent language selection and unify the callsites.

This bug should not depend on bug 1325870 since as :emk pointed out, this patch should not change how we select language.

    
    

    
  
(In reply to Axel Hecht [:Pike] from comment #122)
> general.useragent.locale has little to do with the UI locale.
> Also, navigator.languages has little to do with the UI locale.
> 
> The recommended way to get the UI locale today is:
> 
> Cc['@mozilla.org/chrome/chrome-registry;1'].getService(Ci.
> nsIXULChromeRegistry).getSelectedLocale('global')
> 
> Sadly,
> https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Locale.jsm#20
> uses brittle code paths, too. In particular, you can enter random junk as
> your selected locale in a non-matchOS single-locale build, and it will
> return that.

Wow, thank you for the information.
No longer depends on: 1308329

    
    

    
  
(In reply to Masatoshi Kimura [:emk] from comment #107)
> I backed out this change:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> a6882a49e907bd01ab7b782a0985614f4565e02e
> 
> Probably holiday season is not the good timing to make such large changes.

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/a6882a49e907
Status: REOPENED → ASSIGNED

          status-firefox53:
          fixed → ---
Target Milestone: mozilla53 → ---

        Attachment #8814756 -
        Attachment is obsolete: true

        Attachment #8820509 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #111)
> > The OS knows best what the user wants
> That's your opinion. If you want to participate in the conversation, I'd
> appreciate if you recognized that and notice that other people have a
> different one.
> It would also help if you assumed that their opinion is based on knowledge
> and experience. That makes it easier to talk.
Zibi, could you please elaborate on your opinion that the OS doesn't know best or point me to the comment where you already said so. I've just checked my Android phone. There is a system setting for time and date formats and I would expect all apps to follow that and not to have to change the formats in every app. Why would it be a good thing for Firefox having to configure the formats separately from the OS? If I understand the approach correctly, - say - Indian users should set the locale to en-IN, to get pre-canned Indian formats, but what about those who want to tweak this? I for example prefer 27/12/2016 13:01, which is neither matching my German location (since the Germans use 27.12.2016) nor the the en-AU locale (since I believe the Australians use 12h with AM/PM, https://en.wikipedia.org/wiki/Date_and_time_notation_in_Australia, and there the question is, whether you want this uppercase or lowercase). Actually, it just occurred to me that there are mostly homogeneous locales, like Germany where most people use 27.12.2016 13:01, and heterogeneous locales, like Australia, where half the population uses 12h and the rest 24h. So with all due respect, I don't think the pure ICU/CLDR is the way to go, well, you said yourself in comment #108: "The open challenge still is how to get it to take OS preferences when they exist, but not override CLDR defaults when there are no OS preferences set." Is there an OS that doesn't provide preferences?

Or maybe you want to carry this discussion to dev-platform.

    
    

    
  
I think this is much more of a UX question than a platform question, tbh.

For users for which there's good locale support on the OS level, and that use their OS and their Firefox in the same language, things are probably easy.

In the case of a mismatch, the underlying question here is if people have their OS to their liking or their Firefox to their liking.

I'm afraid we have users in both buckets. Setting myself on a needinfo to gather some data from telemetry.
Flags: needinfo?(l10n)

    
    

    
  
> Why would it be a good thing for Firefox having to configure the formats separately from the OS?

It's a really broad question. Firefox, like any other web browser serves as an user agent and as such, lives on the crossroads between being an app in the host environment, and being a host environment with (web) apps inside it. Think of it more like VirtualBox (but without an OS inside it, just apps launching) than Thunderbird.

There's a pressure on all web browsers to act the same way irrelevant of their host environments (why can't the form style be the same across all web browsers!), there's a pressure on a single web browser to act the same way irrelevant of the host environment (I'm using Firefox on Mac and Windows and I expect it to work the same way!), and there's a pressure for the web browser to follow the host environment preferences (your feedback).
Those needs are not mutually exclusive, but they are hard to accommodate all at the same time, and every now and then in topics like themes, l10n, privacy, UX etc. someone comes with a strong preference toward one of those aspects and questions if there's a reason to not follow what he believes to be the right thing.

> Or maybe you want to carry this discussion to dev-platform.

Definitely better to move it to dev.platform please. This conversation has nothing to do with the implementation of this bug.

    
    

    
  
The biggest outliers between OS and Fx locale are Indonesian OS - English Fx, Arabic - English, Persian - English.

I'd expect arabic dateformats in English strings to be buggy in practice. Interesting they'd put up with it.

For the majority of our userbase, though, OS and Fx match up pretty nicely.
Flags: needinfo?(l10n)

    
    

    
  
Shoot, that's too bad. Is there anything I should look into? Or are we postponing this bug for now? Let me know, thanks.

(In reply to :Cykesiopka from comment #110)
> Review of attachment 8820509 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The indentation here is off.

Thanks for catching that. Not really sure when that got messed up.
Flags: needinfo?(olucafont6) → needinfo?(VYV03354)

    
    

    
  
(In reply to Gregory Moore from comment #130)
> Is there anything I should look into?

Could you debug DateTimeFormatICU to sort out why the user locale does not affect the result?
Flags: needinfo?(VYV03354)

    
    

    
  
(In reply to Jorg K (GMT+1) from comment #104)
> I would like to complain about this:
> 1) This got landed busting Thunderbird big time with no heads-up.

Sorry about the lack of a heads-up. (I believe one isn't required by the rules, but it would have been courteous to notify you anyway.)

> 2) You removed the locale parameter from FormatPRTime() which we were
> actually using.

We shouldn't keep c-c-specific code in m-c. Either c-c should contain the wrapper code to convert PRTime into a representation that m-c can format, or c-c should have its own date formatting code.

The latter is probably preferable, since dates in Firefox show up in fringe UI but dates in Thunderbird show up in central UI.

(In reply to Jorg K (GMT+1) from comment #105)
> Can someone please point out the comment where the firm decision is made and
> agreed that this should go ahead ignoring all user configuration.

In comment 21, gandalf said: "I'm ok with not blocking on it and regressing a bit here". In comment 23, jfkthame agreed with gandalf though not explicitly with the regression acceptance: "I think what gandalf is proposing in the latest comments above makes good sense." In comment 24, I agreed with not blocking this bug on supporting OS-level format customizations.

Speaking for myself only, my main reason for agreeing not to block on OS-level customizations was that the more visible UI in the pipeline (HTML date inputs) would be based on the JS Intl API, which doesn't currently support OS-level customizations, so it seemed silly to require more elegance of the less visible UI (the callers of this C++ API).

As for the use cases:

There are six notable cases:
 1) en-US browser on en-US OS, no customization
 2) en-US browser on non-en-US OS, no customization
 3) non-en-US browser on matching non-en-US OS, no customization
 4) en-US browser on en-US OS, format customized
 5) en-US browser on non-en-US OS, format customized
 6) non-en-US browser on matching non-en-US OS, format customized

#1 and #3 are the easy cases. Potential ICU differences are arguments about aesthetics like "at" in English long format. In my opinion, if you object to "at", you should take the fight to ICU/CLDR level.

#3 offers potential for confusion anyway: If the OS uses slash-separated month and day but not in the U.S. order, there's nothing we can do to make it non-confusing and the user just has learn what happens in which app and live with it.

Of the last three, case #4 is likely more common that cases #5 and #6. Many users have reasons to run non-translated UI strings but still find U.S. 12-hour clock or date format distasteful.

Cases #5 and #6 are likely quite a bit less common (but existing, sure) and probably don't matter nearly as much as #4 *for this API in Firefox* given that C++-formatted dates and times aren't central to the Firefox UI.

They are to Thunderbird UI.

I suggest the way forward is:
 * Thunderbird formats its date in c-c code according to Thunderbird's needs.
 * This patch re-lands for Firefox. After all, most of bug 1301655 already landed, and the bits of UI that now go through the JS Intl API don't pick up OS-level customizations.
 * Once the JS Intl API develops a way to address case #4 above, extend the C++ case to support the same.
Flags: needinfo?(hsivonen)

    
    

    
  
Henri, thank you for your comments.

(In reply to Henri Sivonen (:hsivonen) from comment #132)
> (I believe one isn't required by the
> rules, but it would have been courteous to notify you anyway.)
Yes.

> > 2) You removed the locale parameter from FormatPRTime() which we were
> > actually using.
> We shouldn't keep c-c-specific code in m-c. Either c-c should contain the
> wrapper code to convert PRTime into a representation that m-c can format, or
> c-c should have its own date formatting code.
It's not a big problem losing the locale parameter.

> The latter is probably preferable, since dates in Firefox show up in fringe
> UI but dates in Thunderbird show up in central UI.
Nicely put: Fringe UI vs. central UI ;-)

> There are six notable cases:
>  4) en-US browser on en-US OS, format customized
...
> Of the last three, case #4 is likely more common that cases #5 and #6. Many
> users have reasons to run non-translated UI strings but still find U.S.
> 12-hour clock or date format distasteful.
I'd say this is quite a common case where people don't bother about en-AU/en-IN, etc. (or explicitly want en-US) but still want to see their localised date/time formats.

> I suggest the way forward is:
>  * Thunderbird formats its date in c-c code according to Thunderbird's needs.
Hmm, that would mean "forking" some of the existing code. We currently fully rely on the existing M-C C++ interface.

>  * This patch re-lands for Firefox. After all, most of bug 1301655 already
> landed, and the bits of UI that now go through the JS Intl API don't pick up
> OS-level customizations.
>  * Once the JS Intl API develops a way to address case #4 above, extend the
> C++ case to support the same.
That's the part I don't understand. In bug 1325751 I looked at the "Page Info", and I got |December 25, 2016, 7:23:07 PM| no matter what screws I turned. In the bookmarks management, "Added" and "Last Modified" behave the same (I still have that binary installed).

So are you really saying, the case #4 won't have *any* way to configure their date/time display? I don't think that's acceptable, not even in the "fringe" UI.

Reading comment #114
  IMO we should do it to mitigate the impact of some locales.
comment #116
  We should investigate why this patch no longer respects the operating system locale.
comment #118
  That is, the patch is plain simple broken.
comment #131
  Could you debug DateTimeFormatICU to sort out why the user locale does not affect the result?
I get the impression that the patch shouldn't reland as-is.

I thought there was some consensus that the display should be based on the browser UI locale. As an experiment, I have general.useragent.locale set to de-DE in one profile now and some add-ons suddenly show German UI. So if they are driven by that setting, why shouldn't the dates follow?

    
    

    
  
(In reply to Jorg K (GMT+1) from comment #133)
> As an experiment, I have general.useragent.locale set to
> de-DE in one profile now

As written in comment #122, general.useragent.locale has little to do with the browser UI locale. You will have to install German language pack *and* modify general.useragent.locale to change the browser UI locale. (I tested it locally using 2016-12-24 Nightly.)

> and some add-ons suddenly show German UI.

That's exactly because Firefox uses some inconsistent methods to determine the browser UI locale.

    
    

    
  
(In reply to Masatoshi Kimura [:emk] from comment #134)
> As written in comment #122, general.useragent.locale has little to do with
> the browser UI locale. You will have to install German language pack *and*
> modify general.useragent.locale to change the browser UI locale. (I tested
> it locally using 2016-12-24 Nightly.)
Well, my aim is not to turn FF into a German version, my aim is to adapt the date/time format to use 24h (no AM/PM) and DD/MM/YYYY. And that in an en-US version (use case #4 from comment #132).
There is no en-AU or en-IN language pack, so I can't do that what you suggested. I tried installing en-GB and de-DE but both claimed to be incompatible with FF Nightly. Maybe one has to fiddle with the package to get it to install. Or there is a Nightly package?

I did however install an en-GB FF Nightly of 25/12/2016 and it does show 3 January 2017, 00:19:08 and 02/01/17 23:54 in the bookmarks' "Added" column. I'm not sure that the short YY will make people happy.

I also installed an en-GB TB Daily of 25/12/2016 and it shows 1/12/16 11:42 am, so no leading "0" in the day/month and AM/PM times. As far as I'm aware, TB gets the times via the C++ interface. I still don't understand why localised en-GB FF and TB versions both with the patch here applied and both of 25/12/2016 behave so differently. So are we sure the ICU/CLDR works in the C++ interface?

> > and some add-ons suddenly show German UI.
> That's exactly because Firefox uses some inconsistent methods to determine
> the browser UI locale.
So is there any way to set the browser UI locale without installing a language pack (since there isn't one)?

    
    

    
  
(In reply to Jorg K (GMT+1) from comment #135)
> I also installed an en-GB TB Daily of 25/12/2016 and it shows 1/12/16 11:42
> am, ...
Finally I tried a "de" TB version (there is no "de-DE"). That displays the dates/times as 4/12/16, 10:12 am whereas in German they should be 04.12.2016 22:12. So in TB the date/time display doesn't follow the browser UI locale.

    
    

    
  
(In reply to Jorg K (GMT+1) from comment #135)
> There is no en-AU or en-IN language pack, so I can't do that what you
> suggested. I tried installing en-GB and de-DE but both claimed to be
> incompatible with FF Nightly. Maybe one has to fiddle with the package to
> get it to install. Or there is a Nightly package?

I used
https://ftp.mozilla.org/pub/firefox/nightly/2016/12/2016-12-24-03-02-05-mozilla-central/firefox-53.0a1.en-US.win64.zip
and
https://ftp.mozilla.org/pub/firefox/nightly/2016/12/2016-12-24-03-02-05-mozilla-central-l10n/win64/xpi/firefox-53.0a1.de.langpack.xpi
.

> So is there any way to set the browser UI locale without installing a
> language pack (since there isn't one)?

I don't think it's possible because the browser UI locale is determined based on the selected language pack.

(In reply to Jorg K (GMT+1) from comment #136)
> Finally I tried a "de" TB version (there is no "de-DE"). That displays the
> dates/times as 4/12/16, 10:12 am whereas in German they should be 04.12.2016
> 22:12. So in TB the date/time display doesn't follow the browser UI locale.

That's not necessarily so. ICU/CLDR may not return the appropriate date format for de or de-DE or whatever.

    
    

    
  
(In reply to Masatoshi Kimura [:emk] from comment #137)
> ICU/CLDR may not return the appropriate date
> format for de or de-DE or whatever.

http://www.unicode.org/cldr/charts/30/summary/de.html indicates that CLDR should have day before month and the period between day and month and month and year.

    
    

    
  
> In my opinion, if you object to "at", you should take the fight to ICU/CLDR level.

I agree with :hsivonen on this.

    
    

    
  
Just to confirm:
A German localised Thunderbird from
http://ftp.mozilla.org/pub/thunderbird/nightly/2016/12/2016-12-25-03-02-04-comm-central-l10n/thunderbird-53.0a1.de.win32.zip
shows 20/9/16, 12:51 am

The same for an en-US version from
http://ftp.mozilla.org/pub/thunderbird/nightly/2016/12/2016-12-25-03-02-04-comm-central/thunderbird-53.0a1.en-US.win32.zip
with German language pack from
http://ftp.mozilla.org/pub/thunderbird/nightly/2016/12/2016-12-25-03-02-04-comm-central-l10n/win32/xpi/thunderbird-53.0a1.de.langpack.xpi
and setting general.useragent.locale to de.

That would suggest that the C++ interface isn't returning the right thing.

I also tried a German localised *Firefox* from here
http://ftp.mozilla.org/pub/firefox/nightly/2016/12/2016-12-25-03-02-06-mozilla-central-l10n/firefox-53.0a1.de.win32.zip
to look for a spot where C++ generated dates/times are displayed. Studying the patch to find C++ call sites I spotted a few in security/manager/ssl.

Looking at some certificates the German FF displays:
Gültigkeit, Beginnt mit (Validity starts)
15 March 2011
And under details:
Validität, Nicht vor (Validity, not before)
15 March 2011 at 1:00:00 am
If I'm not mistaken, that's the same non-localised format in a fringe location that shows in TB in a central location.

So unless that's hard-coded into the certificate, I'd suggest again that the C++ interface implemented here is not correct and I'd request it to be fixed before relanding so that Germans, the most faithful FF (and TB) users, get a consistent product and/or people can at least influence the date/time format by installing a language pack.

If you land this patch again unchanged, all TB users worldwide, regardless of which version or language pack they have installed, will see en-US formats. I know you don't care much about this, but perhaps you care about the correctness of the Mozilla core C++ interface.

    
    

    
  
(In reply to Jorg K (GMT+1) from comment #140)
> I also tried a German localised *Firefox* from here
> http://ftp.mozilla.org/pub/firefox/nightly/2016/12/2016-12-25-03-02-06-
> mozilla-central-l10n/firefox-53.0a1.de.win32.zip
> to look for a spot where C++ generated dates/times are displayed. Studying
> the patch to find C++ call sites I spotted a few in security/manager/ssl.
> 
> Looking at some certificates the German FF displays:
> Gültigkeit, Beginnt mit (Validity starts)
> 15 March 2011
> And under details:
> Validität, Nicht vor (Validity, not before)
> 15 March 2011 at 1:00:00 am

Thank you. Indeed that's not what should be displayed. Clearly there is a bug somewhere that needs to be fixed before re-landing.

    
    

    
  
I looked into this a little further. From what I can tell, the way we currently get the application locale will always return "en-us":
https://dxr.mozilla.org/mozilla-central/source/intl/locale/mac/nsDateTimeFormatMac.cpp#45

I tried changing the general.useragent.locale preference using the call:
mozilla::Preferences::SetCString("general.useragent.locale", "fr-fr");
Then when I called:
mozilla::Preferences::GetLocalizedCString("general.useragent.locale", &applicationLocale);
It would return "fr-fr", but if I got the application locale from localeService->GetApplicationLocale(), it would still return "en-us".

I also tried getting the locale this way:
https://dxr.mozilla.org/mozilla-central/source/dom/encoding/FallbackEncoding.cpp#68
This is is similar to what :Pike talks about in comment 122, as the correct way to get the UI locale. However, this still returned "en-us". 

I guess this makes sense because bug 1325870 is about how there's a bunch of different ways to get the locale. The thing is, this is how the original code got the locale. So why wouldn't it work anymore? Wouldn't it have always just returned "en-us"? Actually, before it had the option of passing in a locale. But it doesn't seem like that was used very much, so it shouldn't matter that much. 

So I'm also not really sure how we can effectively change the application locale. I don't know any other ways to change it besides the preferences general.useragent.locale and intl.locale.matchOS, although :Pike said those had little to do with the UI locale anyway. Do I have to install Firefox in a different language, or install a different language pack or something? 

Perhaps localeService->GetApplicationLocale() and the registry->GetSelectedLocale() ways are just getting the locale from the operating system? But bug 1325751 says they don't... So I'm not really sure why it's not working. Let me know if you guys have any ideas, thanks.
Flags: needinfo?(VYV03354)

    
    

    
  
(In reply to Gregory Moore from comment #142)
> Do I have to install Firefox in
> a different language, or install a different language pack or something? 
From what I gather from comment #134, you need to install a language pack and set general.useragent.locale.

I assume you have a local debug build. Why don't you install a German language pack from
https://ftp.mozilla.org/pub/firefox/nightly/2017/01/2017-01-xx-yy-zz-tt-mozilla-central-l10n/win64/xpi/firefox-53.0a1.de.langpack.xpi (or win32 if your build is 32bit, replace xx, yy, zz, tt) and set general.useragent.locale to "de" and see what happens ;-)

    
    

    
  
I applied the patch from this bug in my local 64bit build and I installed
https://ftp.mozilla.org/pub/firefox/nightly/2017/01/2017-01-04-03-02-14-mozilla-central-l10n/win64/xpi/firefox-53.0a1.de.langpack.xpi
and set general.useragent.locale to "de".

That gives a pretty German Firefox, however, as pointed out in comment #140, certificate information is still displayed in English.

I added debug to DateTimeFormatICU.cpp and that prints
=== DateTimeFormat::Initialize() - retrieved en-AU
when inspecting the details of a certificate.

Oh, en-AU, that's my Windows (system) locale. So all that langpack stuff and setting general.useragent.locale was in vain.

Next, I went to my Windows control panel and set the locale to "German (Germany)". After a restart my debug is:
=== DateTimeFormat::Initialize() - retrieved en-AU

Actually, that's no typo, en-AU is *still* printed with:
    rv = localeService->GetApplicationLocale(getter_AddRefs(appLocale));
    if (NS_SUCCEEDED(rv)) {
      rv = appLocale->GetCategory(NS_LITERAL_STRING("NSILOCALE_TIME"), localeStr);
      if (NS_SUCCEEDED(rv) && !localeStr.IsEmpty()) {
        *mLocale = NS_LossyConvertUTF16toASCII(localeStr); // cache locale name
        printf("=== DateTimeFormat::Initialize() - retrieved %s\n", (*mLocale).get());
      }
    }

I checked my settings again and I still had "English (Australia)" in my Windows "Region and Language / Format" settings. So OK, I changed this to "German (Germany)" and now my debug returns:
=== DateTimeFormat::Initialize() - retrieved de-DE
Hooray. And now the date is shown as "31. Juli 2038", which is German.

Next, I uninstalled the German Language pack again, so I have an English-looking FF again. general.useragent.locale still at "de".
Debug:
=== DateTimeFormat::Initialize() - retrieved de-DE
Certificate date: 31. Juli 2038 (German).

Next step: general.useragent.locale back to en-US. Same result as before.

So in summary: It makes no difference what language pack you have installed or what general.useragent.locale is set to. On Windows you get the system defined format not for the "system locale" but for the locale/country defined in "Region and Language / Format".
Note that Windows 7 has a configuration for the "system locale" under "Region and Language / Administrative".

So finally the last attempt with Thunderbird of 25/12/2016 which has this patch:

With the Windows format set to German I get - 29.12.16 23:17
with the Windows format set to English/AU I get - 29/12/16 11:17 pm, so someone decided that Australians want 12h + AM/PM
with the Windows format set to English/GB I get - 29/12/2016 23:17.
with the Windows format set to English/US I get - 12/29/16 11:17 PM

So as far as I'm concerned, the patch can land again "as is" since at least on Windows the user still has the chance to set the format via - let's call it - the "format locale".

I'm really terribly sorry about all the noise caused. It is true that this patch caused bug 1325751 as expected, that means, individual format definitions are ignored. However, with the patch, at least on Windows, the "format locale" is retrieved and used, which seems good enough.

My confusion (and I think other people's confusion) came about when general.useragent.locale and language pack installation got thrown into the mix. That Windows calls one thing "system locale" but configures formats elsewhere also doesn't help.

I was clearly wrong when claiming that the C++ interface wasn't correct. Well, "correct" seems to be a question of definition, since apparently no-one knew where the locale for the C++ interface came from. Now we do ;-)
Flags: needinfo?(VYV03354)

    
    

    
  
Awesome! Thanks for figuring that out. I was trying a bunch of stuff but couldn't figure out how to get localeService->GetApplicationLocale() to return anything other than "en-US", even when I changed my system locale.

I tried replacing the code that gets the locale in DateTimeFormatICU.cpp with the code from:
https://dxr.mozilla.org/mozilla-central/source/dom/encoding/FallbackEncoding.cpp#68
Before I was just running a gtest file and that way would still return "en-US" as well. However, if I actually run firefox, and I have general.useragent.locale set to "de" and the German language pack installed, it will format the security certificate times as German as well. Here's what it they look like (with all my locale settings on Windows set to "en-US"):

31. Oktober 2016   /   31. Oktober 2016 um 17:00:00
18. Dezember 2017   /   18. Dezember 2017 um 04:00:00 

Not sure if we should go with this way or the way we had it before. This way might make more sense as then the times for security certificates would be formatted using the same locale as the other times. Let me know what you guys think, thanks. After we figure it out I can make a new patch.

    
    

    
  
(In reply to Gregory Moore from comment #145)
> Not sure if we should go with this way or the way we had it before. This way
> might make more sense as then the times for security certificates would be
> formatted using the same locale as the other times. Let me know what you
> guys think, thanks. After we figure it out I can make a new patch.

Yeah, but it will break the en-US browser + indic operating system use case as Jorg.K mentioned. Also, I think it would be better to change the localeService->GetApplicationLocale() implementation or add a new method. It would deserve a new separate bug.

As of now, I'll reland the patch as-is as soon as I go home. Jorg, please prepare to reland the corresponding c-c change.
Flags: needinfo?(jorgk)

    
    

    
  
(In reply to Masatoshi Kimura [:emk] from comment #146)
> Jorg, please prepare to reland the corresponding c-c change.
Ready when you are ;-)
Flags: needinfo?(jorgk)

    
    

    
  
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f20e89eb486
Replaced per-platform variants of nsIDateTimeFormat with single, ICU-backed version that has static c++ callers. r=emk

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/0f20e89eb486
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 8 years ago

          status-firefox53:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

    
    

    
  
Cool, glad this bug is finally finished. Thanks for all the help from everyone; I learned a lot from working on this bug. Also, if anyone knows of any other bugs that would be a good next bug, please let me know, as I am looking for some.

Also, unfortunately I never fixed the formatting error mentioned in comment 110. Should I make another patch that just fixes that? Or should we even fix that within this bug? Let me know, thanks.
Flags: needinfo?(VYV03354)

    
    

    
  
(In reply to Gregory Moore from comment #150)
> Also, unfortunately I never fixed the formatting error mentioned in comment
> 110. Should I make another patch that just fixes that? Or should we even fix
> that within this bug? Let me know, thanks.

Oh, sorry for forgetting the format error fix. In general, it is better to file a new followup bug. Bat the patch would be simple enough to attach in this bug, so feel free to attach the followup patch here and request review.
Flags: needinfo?(VYV03354)

    
    

    
  


  
Cool, sounds good. Here it is, thanks.

        Attachment #8824625 -
        Flags: review?(VYV03354)

    
    

    
  
Comment on attachment 8824625 [details] [diff] [review]
Fixed small formatting error in nsNSSCertValidity.cpp introduced by previous patch.

Thank you!

        Attachment #8824625 -
        Flags: review?(VYV03354) → review+

    
    

    
  
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f417be4dd3d
Fixed small formatting error in nsNSSCertValidity.cpp introduced by previous patch. r=emk

    
    

    
  
(In reply to Jorg K (GMT+1) from comment #156)
> Can you check other uses:
> http://searchfox.org/mozilla-central/search?q=kDateFormatYearMonth&case=false&regexp=false&path=
> http://searchfox.org/mozilla-central/search?q=kDateFormatWeekday&case=false&regexp=false&path=

The search result has nsDateTimeFormatUnix.cpp, nsDateTimeFormatMac.cpp, and nsDateTimeFormatWin.cpp. But they are removed by this patch. Is searchfox using the latest source?
Flags: needinfo?(VYV03354)

    
    

    
  
Sure.  Gregory, are you interested in writing a patch?

    
    

    
  
Yeah, I can definitely write the patch.

Also though, I think the reason we got rid of those format selectors was because we thought there were no callers (see comment 3 and comment 36). Since it seems like Thunderbird was still using them, I'm wondering if maybe we should add functionality for those selectors back in. Maybe not though; I'm not sure how important having those options is.
Flags: needinfo?(olucafont6)

    
    

    
  
kDateFormatYearMonth, kDateFormatWeekday were used in TB I removed them for now
https://hg.mozilla.org/comm-central/rev/65c295598322d6863a3f6174de512c384ca19443#l1.12
since they caused a blank date column.

Can you put support back? Does ICU support this? Here
https://dxr.mozilla.org/comm-central/rev/0d823cf54df53e0cea75a74adebace956bd333d8/mozilla/intl/locale/DateTimeFormatICU.cpp#82
I can only see three types: long, short, none.

I don't know how useful these two are, I didn't even know they existed until someone complained on the weekend that they didn't work any more.
Blocks: 1329841

    
    

    
  
I filed bug 1329841. The patch would be too large to continue the work in a closed bug.

    
    

    
  
Nice to see that this stayed in the tree. Thank you for fixing this!

    
  
Depends on: 1331466

    
    

    
  
Like mentioned in comment 98, I don't see a need for web developer documentation here. Feel free to re-add ddn with an explanation of what is worth documenting or updating on MDN.
Keywords: dev-doc-needed
Depends on: 1344594
Blocks: 1349855

    
    

    
  
I was pointed to this bug from http://forums.mozillazine.org/viewtopic.php?f=38&t=3030451

Seems like Firefox 53 now prints the year with 2 digits instead of 4 digits in the header/footer like it used to.  My Windows 7  Pro is set to English - United States with both the Short and Long dates set to 4 digit years.  In the Firefox Page Setup - Margins and Headers, I am using the Date/Time drop-down option on the right-footer. Not 100% sure if this is related to this bug or not. Thanks.
Depends on: 1362817
Blocks: 1400463
No longer blocks: 1400463
Depends on: 1400463

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: