1301655 - Replace usage of nsIScriptableDateFormat in UI JS code with the ECMAScript Intl API

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Henri Sivonen (:hsivonen)]

We shouldn't need to have an XPCOM API like nsIScriptableDateFormat for browser UI JS to format dates and times with. Now that the ECMAScript Intl API is available, UI JS code should use the ECMAScript Intl API instead of nsIScriptableDateFormat so that the nsIScriptableDateFormat legacy API could be removed.

Comment 1

[Jonathan Kew [:jfkthame]]

I'm not sure we even need "a helper around the ECMAScript Intl API" (as per bug title), though it depends on exactly what results we want to see.

The simplest thing here would be to just use Intl.DateTimeFormat() directly wherever front-end code currently uses nsIScriptableDateFormat. In some cases, this would result in a slightly different date/time format than we currently get, but is that important? I think not; the results we get by passing appropriate options to Into.DateTimeFormat() should be perfectly acceptable.

The main behavior difference from current code would be that currently, nsIScriptableDateFormat is backed by separate platform-specific implementations that in some cases yield substantially different results for the same locale. This shows up as platform-dependent date/time formatting in various places in the UI.

For example, consider the "Modified" date shown in the Page Info dialog. Depending on the OS and the current system locale, I see forms such as:

  October 18, 2016 at 3:27:13 PM         (Mac OS X, en-US)
  18 October 2016 at 15:27:13            (Mac OS X, en-GB)
  Tue 18 Oct 2016 03:27:13 BST           (Ubuntu, en-US)
  Tue 18 Oct 2016 15:27:13 BST           (Ubuntu, en-GB)
  Tuesday, October 18, 2016 3:27:13 PM   (Win10, en-US)
  18 October 2016 15:27:13               (Win10, en-GB)

Moreover, the user can customize these using a system-level control/settings panel (at least on OS X and Windows; I'm not sure what user-level configuration tools are typically offered for this on Linux), and the Firefox UI will reflect those customizations.

If we replace the use of nsIScriptableDateFormat there with Intl.DateTimeFormat, we'll get a system-independent (but still system-locale-dependent) result:

  October 18, 2016, 3:27:13 PM           (Intl API, en-US)
  18 October 2016, 15:27:13              (Intl API, en-GB)

and these will NOT be affected by user customizations at the OS level.


Once we have bug 1308329, we could make these formats respond if the user changes the 12/24-hr preference from its default for their locale. We still won't be able to make them completely follow the OS format, however, because AFAICS the Intl.DateTimeFormat API doesn't currently support arbitrary customization of the formatting pattern, it just creates/selects a pattern internally from a higher-level and less flexible set of options.

Comment 2

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

> Once we have bug 1308329, we could make these formats respond if the user changes the 12/24-hr preference from its default for their locale. We still won't be able to make them completely follow the OS format, however, because AFAICS the Intl.DateTimeFormat API doesn't currently support arbitrary customization of the formatting pattern, it just creates/selects a pattern internally from a higher-level and less flexible set of options.

Filed https://github.com/tc39/ecma402/issues/108 for this.

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)
> Filed https://github.com/tc39/ecma402/issues/108 for this.

Should we wait for a conclusion there before we move forward here, or is it OK to go ahead and remove the nsIScriptableDateFormat usage, accepting that we'll lose user customization for now, and then have a followup to support user-customized OS prefs once the appropriate mechanism has been designed?

Comment 4

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

I believe it's ok to lose customization in this pathway for now, and fix it globally through consensus in ECMA402.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - Replace use of nsIScriptableDateFormat with standard JS date formatting in pageInfo.js

OK, so for now, I think it should be OK to do changes like this wherever nsIScriptableDateFormat is currently used in front-end JS. If we work through each of the call sites doing this kind of replacement, we should eventually be able to remove the nsIScriptableDateFormat interface and implementation altogether (though we should announce that intention ahead of time, as it may also be used by add-ons, etc.)

Comment 6

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

Comment on attachment 8802465 [details] [diff] [review]
pt 1 - Replace use of nsIScriptableDateFormat with standard JS date formatting in pageInfo.js

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

One potential nit, but overall, r+! :)

::: browser/base/content/pageinfo/pageInfo.js
@@ +1058,5 @@
>      return unknown;
>  
> +  const dtOptions = { year: 'numeric', month: 'long', day: 'numeric',
> +                      hour: 'numeric', minute: 'numeric', second: 'numeric' };
> +  return date.toLocaleString([], dtOptions);

Seems like we want to keep using
```
Cc["@mozilla.org/chrome/chrome-registry;1"].getService(Ci.nsIXULChromeRegistry).getSelectedLocale("global")
```

to retrieve UI locale for now.

I'm working with Axel and Stas on redefinition of UI locale fallback chain and API for that, and we'll migrate all callsites from this to the new one once we establish that.

For now, seems like for consistency reasons we should use this.

Comment 7

[Jonathan Kew [:jfkthame]]

Are you sure we want that? AFAICS, that will give us the UI locale of the browser; but what I see in a current Nightly is that the date/time here follows the *system* locale, not the browser UI language. So if I switch my OS X locale from en-GB to en-US, for example, it adjusts the date format accordingly; and if I set my locale to de-DE then I see German dates, etc. All while running the same (English) build.

Whether it's better to follow the browser's or system's language may be debatable, but AFAICT we'll stay closer to current behavior if we pass [] for the locale list here and let ICU get the system default.

Comment 8

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

I *think* we want to keep our UI locale, and not necessarily follow OS locale. Checking with :pike.

Comment 9

[Axel Hecht [:Pike]]

In particular for our UI, I think we should adhere to our UI locale.

I remember a small but steady stream of bugs over the years where people complained about the OS locale leaking weirdly into their UI.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - Replace use of nsIScriptableDateFormat with standard JS date formatting in pageInfo.js

OK, let's go with that then. Here's the updated patch that should use the UI locale.

Comment 11

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

Comment on attachment 8802685 [details] [diff] [review]
pt 1 - Replace use of nsIScriptableDateFormat with standard JS date formatting in pageInfo.js

lgtm!

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - Replace use of nsIScriptableDateFormat with Intl.DateTimeFormat in FeedWriter.js

Looking at another place where we use nsIScriptableDateFormat in browser code... we could do essentially the same thing here as for pageInfo.js, but considering that in FeedWriter the _parseDate() function is called in a loop, it seems worth caching a DateTimeFormat object for re-use rather than repeatedly looking up the locale and calling Date.toLocaleString() each time.

Comment 13

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

Comment on attachment 8802941 [details] [diff] [review]
pt 2 - Replace use of nsIScriptableDateFormat with Intl.DateTimeFormat in FeedWriter.js

r+

Once we switch away from the current way of retrieving UI locale, we'll want to start observing the language change event and reset the cache here, but it's easy to find it with a query for all uses of the getSelectedLocale, and we don't react to language changes right now anywhere.

Comment 14

[Jonathan Kew [:jfkthame]]

There's an additional complication here. I believe the patches above will fail on the OS X Japanese build, because we use a special non-standard locale code "ja-JP-mac" there; that's what will be returned by getSelectedLocale("global").

This is not a well-formed BCP47 tag, and will fail the IsStructurallyValidLanguageTag() operation in ecma-402; and that means that when we pass it to the Intl API functions (e.g. through toLocaleString(locale, ...)), they will throw a RangeError.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: pt 0 - Add an optional parameter to nsIChromeRegistry.getSelectedLocale to allow callers to request the locale be returned as a valid BCP47 lang tag

Here's a possible way to address the ja-JP-mac issue. With this patch, we can then pass 'true' for the asBCP47 parameter when we want to pass the result to Intl API functions. Existing callers that don't pass that parameter will be unaffected. An alternative approach would be to have a separate getSelectedLocaleAsBCP47 method; any preference for which way we should do it?

Comment 16

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

Comment on attachment 8803370 [details] [diff] [review]
pt 0 - Add an optional parameter to nsIChromeRegistry.getSelectedLocale to allow callers to request the locale be returned as a valid BCP47 lang tag

Can you please add a comment right above "uloc_toLanguageTag" that says:

'This is a fail-safe method that returns "und" if it cannot match any part of the language tag.'

Most ICU methods are not fail-safe, so I was actually surprised to see that.

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: pt 0 - Add an optional parameter to nsIChromeRegistry.getSelectedLocale to allow callers to request the locale be returned as a valid BCP47 lang tag

With added comment as requested; carrying forward r=gandalf. I'll update the front-end patches to use this, so that they won't mysteriously fail on the Mac Japanese version.

Comment 18

[Jonathan Kew [:jfkthame]]

I have a series of patches to replace nsIScriptableDateFormat in additional parts of our code. We may need to check whether any of this code is used in the Android build (I'm not sure offhand -- some, at least, is desktop-only); if it is, then we may not be able to land these patches on m-c until the Intl API is enabled there (presumably when larch is merged).

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: pt 3 - Replace use of nsIScriptableDateFormat with Intl API methods in browser/components/places code

Comment 20

[Jonathan Kew [:jfkthame]]

Attachment: pt 4 - Replace use of nsIScriptableDateFormat with Date.toLocaleString in toolkit Update history

Comment 21

[Jonathan Kew [:jfkthame]]

Attachment: pt 5 - Replace use of nsIScriptableDateFormat for cookie date formatting

Comment 22

[Jonathan Kew [:jfkthame]]

Attachment: pt 6 - Replace use of nsIScriptableDateFormat in download-utils

Comment 23

[Jonathan Kew [:jfkthame]]

Attachment: pt 7 - Replace use of nsIScriptableDateFormat in crashreporter

Comment 24

[Jonathan Kew [:jfkthame]]

Attachment: pt 8 - Replace use of nsIScriptableDateFormat in toolkit/extensions code

Comment 25

[Jonathan Kew [:jfkthame]]

Attachment: pt 9 - Update test_bug22310.js to remove use of nsIScriptableDateFormat

Comment 26

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

Comment on attachment 8804702 [details] [diff] [review]
pt 3 - Replace use of nsIScriptableDateFormat with Intl API methods in browser/components/places code

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

I'm wondering if it would make sense to define some simple defaults for dates/times that we'll be using, sth like:

mozIntl.DateOptions = {
  'short': {},
  'medium': {}.
  'long': {}
};

and use them everywhere (until we get this into ECMA :)).

::: browser/components/places/content/treeView.js
@@ +502,3 @@
>  
> +  // We use a different formatter for times within the current day,
> +  // so we cache both a "today" formatter and a general date formatter.

Would you want to set a cache invalidation on language change, or do you think it's ok to do this en-bulk for all Intl API callsites when we'll be turning on language switching?

Comment 27

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

Comment on attachment 8804704 [details] [diff] [review]
pt 5 - Replace use of nsIScriptableDateFormat for cookie date formatting

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

::: toolkit/components/cookie/content/cookieAcceptDialog.js
@@ +178,5 @@
>  
>      // if a server manages to set a really long-lived cookie, the dateservice
>      // can't cope with it properly, so we'll just return a blank string
>      // see bug 238045 for details
> +    // XXX Is this still a concern after switching to Date.toLocaleString()?

This code works properly with 12 digit stamps like "253407484806" from the bug. I believe you can clean up this try.

Comment 28

[Jonathan Kew [:jfkthame]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #26)
> Would you want to set a cache invalidation on language change, or do you
> think it's ok to do this en-bulk for all Intl API callsites when we'll be
> turning on language switching?

I'm inclined to do that all at once (along with testing that it actually works) when we're ready for it.

Comment 29

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

Comment on attachment 8804714 [details] [diff] [review]
pt 9 - Update test_bug22310.js to remove use of nsIScriptableDateFormat

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

why not remove this test lines?

Comment 30

[Jonathan Kew [:jfkthame]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #26)
> I'm wondering if it would make sense to define some simple defaults for
> dates/times that we'll be using, sth like:
> 
> mozIntl.DateOptions = {
>   'short': {},
>   'medium': {}.
>   'long': {}
> };
> 
> and use them everywhere (until we get this into ECMA :)).

I wondered about that, too, but wasn't sure it was worth it; the direct use of toLocalString(...) with an explicit set of options at the callsite seems pretty clear and readable to me. And we may have cases that don't fit any of those "standard" patterns anyway; e.g. there's at least one place where we format a date as year+month, which we'd still have to do with explicit options.

Comment 31

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

I agree that we don't need it and I don't even think it's that important for code readability.

But I do believe that defining convention will make it easier for general UX consistency as it'll become more likely that developers will just take the right date format instead of mingling with options which in result should reduce the number of date/time combinations that users are exposed to and in case we succeed at getting style in ECMA402, it'll be easier to switch.

But I'm ok with doing it later and not blocking those changes on it :)

Comment 32

[Jonathan Kew [:jfkthame]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #29)
> Comment on attachment 8804714 [details] [diff] [review]
> pt 9 - Update test_bug22310.js to remove use of nsIScriptableDateFormat
> 
> Review of attachment 8804714 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> why not remove this test lines?

The obsolete ja-JP-mac entry? Sure, might as well.

Comment 33

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #22)
> Created attachment 8804706 [details] [diff] [review]
> pt 6 - Replace use of nsIScriptableDateFormat in download-utils

This (alone of the patches above) appears to be part of the Android product, so if we land it on central before bug 1215247 is resolved, it will significantly affect the display of dates in the Downloads list (about:downloads). If I understand correctly, it will mean dates in the list will all be shown in the default (short) format of toLocaleDateString(), instead of the long-month + day format we're currently using.

:gandalf, do you think we should just accept that as a regression for now, or hold off on landing this patch until Date.toLocaleDateString() on Android can handle the new Intl options argument?

Comment 34

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

My gut feeling is that we should wait with this one patch. Is it the last place where we're using the nsIScriptableDateFormat?

Also, I'm not sure how it works right now. Which platform's nsDateFormat*.cpp is it using now on android?

Comment 35

[Jonathan Kew [:jfkthame]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #34)
> My gut feeling is that we should wait with this one patch.

Yeah, I tend to agree.

> Is it the last
> place where we're using the nsIScriptableDateFormat?
> 

It's close, but IIRC there are still a few more occurrences -- though maybe only in its own tests, which we can just kill.

I'm thinking I should post an "intent to unship" to dev-platform, to give people a heads-up that we'd like to get rid of it altogether. There are some callers in comm-central, and I'd expect it's also used by some add-ons etc., so it's only fair to give them some warning.

> Also, I'm not sure how it works right now. Which platform's
> nsDateFormat*.cpp is it using now on android?

When it uses nsIScriptableDateFormat, I believe that'll be using nsDateTimeFormatUnix, which means the implementation ends up choosing a format pattern and calling strftime to do the work. But the code here also has a couple of cases where instead of one of the nsIScriptableDateFormat formats, it uses Date.toLocaleFormat() directly (passing it a pattern of "%A", "%B" or "%d") to get individual parts of the date. I'm not sure where the implementation of that lives.

(Don't know why it's written that way, given that nsIScriptableDateFormat offers dateFormatYearMonth and dateFormatWeekday; AFAICS we never actually use those at all, because the code here does it manually.)

Comment 36

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

Yeah, intend to unship sounds like the right thing to do, we may want to put deprecation warning on it as well for a while.

Comment 37

[Jonathan Kew [:jfkthame]]

Morphing this bug slightly to make it clear it is about changing the existing users in our UI JS code; I'll file a separate bug about deprecating and removing the interface itself.

Comment 38

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6ab66ffae99eed12351041efd822efc6ae18858
Bug 1301655 - pt 0 - Add an optional parameter to nsIChromeRegistry.getSelectedLocale to allow callers to request the locale be returned as a valid BCP47 lang tag. r=gandalf

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd3bd7aa413d765587d94db7a6fc8b10c4008af6
Bug 1301655 - pt 1 - Replace use of nsIScriptableDateFormat with standard JS date formatting in pageInfo.js. r=gandalf

https://hg.mozilla.org/integration/mozilla-inbound/rev/625ba7fc4b96abe8474f5f478b39692e72dd1233
Bug 1301655 - pt 2 - Replace use of nsIScriptableDateFormat with Intl.DateTimeFormat in FeedWriter.js. r=gandalf

https://hg.mozilla.org/integration/mozilla-inbound/rev/68543a46ef49fb7309c345179c87912b8d75e41b
Bug 1301655 - pt 3 - Replace use of nsIScriptableDateFormat with Intl API methods in browser/components/places code. r=gandalf

https://hg.mozilla.org/integration/mozilla-inbound/rev/59f24ab7fc079e573cbdb2fb8d828a951639e62a
Bug 1301655 - pt 4 - Replace use of nsIScriptableDateFormat with Date.toLocaleString in toolkit Update history. r=gandalf

https://hg.mozilla.org/integration/mozilla-inbound/rev/48e81da5a95b8b2c53213a3675ce20d86df87a35
Bug 1301655 - pt 5 - Replace use of nsIScriptableDateFormat for cookie date formatting. r=gandalf

https://hg.mozilla.org/integration/mozilla-inbound/rev/17efbf713de8bb8f672340ef87cf0e78e5565232
Bug 1301655 - pt 7 - Replace use of nsIScriptableDateFormat in crashreporter. r=gandalf

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e2d25bd70b8412864e5005ea4099eb53f183d5c
Bug 1301655 - pt 8 - Replace use of nsIScriptableDateFormat in toolkit/extensions code. r=gandalf

https://hg.mozilla.org/integration/mozilla-inbound/rev/58dbf93c50dafeadab27fced867ab2b2717e57db
Bug 1301655 - pt 9 - Update test_bug22310.js to remove use of nsIScriptableDateFormat. r=gandalf

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6ab66ffae99
pt 0 - Add an optional parameter to nsIChromeRegistry.getSelectedLocale to allow callers to request the locale be returned as a valid BCP47 lang tag. r=gandalf
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd3bd7aa413d
pt 1 - Replace use of nsIScriptableDateFormat with standard JS date formatting in pageInfo.js. r=gandalf
https://hg.mozilla.org/integration/mozilla-inbound/rev/625ba7fc4b96
pt 2 - Replace use of nsIScriptableDateFormat with Intl.DateTimeFormat in FeedWriter.js. r=gandalf
https://hg.mozilla.org/integration/mozilla-inbound/rev/68543a46ef49
pt 3 - Replace use of nsIScriptableDateFormat with Intl API methods in browser/components/places code. r=gandalf
https://hg.mozilla.org/integration/mozilla-inbound/rev/59f24ab7fc07
pt 4 - Replace use of nsIScriptableDateFormat with Date.toLocaleString in toolkit Update history. r=gandalf
https://hg.mozilla.org/integration/mozilla-inbound/rev/48e81da5a95b
pt 5 - Replace use of nsIScriptableDateFormat for cookie date formatting. r=gandalf
https://hg.mozilla.org/integration/mozilla-inbound/rev/17efbf713de8
pt 7 - Replace use of nsIScriptableDateFormat in crashreporter. r=gandalf
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e2d25bd70b8
pt 8 - Replace use of nsIScriptableDateFormat in toolkit/extensions code. r=gandalf
https://hg.mozilla.org/integration/mozilla-inbound/rev/58dbf93c50da
pt 9 - Update test_bug22310.js to remove use of nsIScriptableDateFormat. r=gandalf

Comment 40

[Jonathan Kew [:jfkthame]]

Marking as leave-open because patch 6 has not yet landed here (waiting for bug 1215247 to be resolved first).

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b6ab66ffae99
https://hg.mozilla.org/mozilla-central/rev/cd3bd7aa413d
https://hg.mozilla.org/mozilla-central/rev/625ba7fc4b96
https://hg.mozilla.org/mozilla-central/rev/68543a46ef49
https://hg.mozilla.org/mozilla-central/rev/59f24ab7fc07
https://hg.mozilla.org/mozilla-central/rev/48e81da5a95b
https://hg.mozilla.org/mozilla-central/rev/17efbf713de8
https://hg.mozilla.org/mozilla-central/rev/8e2d25bd70b8
https://hg.mozilla.org/mozilla-central/rev/58dbf93c50da

Comment 42

[Magnus Melin [:mkmelin]]

(In reply to Axel Hecht [:Pike] from comment #9)
> In particular for our UI, I think we should adhere to our UI locale.
> 
> I remember a small but steady stream of bugs over the years where people
> complained about the OS locale leaking weirdly into their UI.

Admittedly it's a bit of a hard topic. What I think is pretty clear is that people UI that's not mixed language (e.g. "Tuesday"). But at the same time it's crucial time *formatting* is what the OS gives you. It's pretty common to run an en-US application but, under no circumstances do you want to figure out what 7/7/10 means for a date there. You have to be able to trust that's formatted as your OS would format times.

Comment 43

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

> But at the same time it's crucial time *formatting* is what the OS gives you

I respect your opinion. Please, recognize that it is not "obvious", and "crucial". It's just your opinion.

Web Browser is a fairly specific type of application that aims to act as an user agent, and share behavior between different host environments. That creates a set of challenges like this one.
Following host OS date time formatting vs. CLDR locale (app locale) date-time formatting.

Comment 44

[Makoto Kato [:m_kato]]

Since I have landed bug 1215247, you can land part 6.  But this fix will conflict with bug 1200494 (landed in autoland)

Comment 45

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

There should be no more uses of nsIScriptableDateFormat in the UI.

Comment 46

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.