Issue 5278 - Improved date formatting approach, in particular fixing broken formatting in 32-bit Li…

Issue 5278 - Improved date formatting approach, in particular fixing broken formatting in 32-bit Linux builds

[Wladimir Palant, 2017-06-08 08:13:02]

https://codereview.adblockplus.org/29459562/diff/29459563/lib/utils.js
File lib/utils.js (right):

https://codereview.adblockplus.org/29459562/diff/29459563/lib/utils.js#newcod...
lib/utils.js:249: * @return {String} formatted date and time
Yes, I am aware of this not being entirely correct. Not going to polish this
however, with the entire code base being deprecated.

https://codereview.adblockplus.org/29459562/diff/29459563/lib/utils.js#newcod...
lib/utils.js:255: return Utils.dateFormatter.format(time);
The MDN documentation doesn't define the parameter here properly, with the
examples suggesting that Date objects are expected. However, the spec says that
the implementation needs to call ToNumber() on the parameter - so a numerical
representation of the date is perfectly fine.

https://codereview.adblockplus.org/29459562/diff/29459563/lib/utils.js#newcod...
lib/utils.js:564: return new Intl.DateTimeFormat(Utils.appLocale, {
Originally, I meant to pass undefined as first parameter here, which should make
the browser fall back to the default locale (supposedly, system's locale).
However, this produced inconsistent results on my system which uses a mixture of
English and German locale settings (LANG=en_US.UTF-8, LC_TIME=de_DE.UTF-8).
Firefox 53 used the German date format whereas Firefox 55 and Chrome chose the
US format. So I thought that a date format matching our UI language might be
more consistent. Not really sure about the expectations of somebody using an
English-language browser however.

[Wladimir Palant, 2017-06-08 08:49:02]

https://codereview.adblockplus.org/29459562/diff/29459563/lib/utils.js
File lib/utils.js (right):

https://codereview.adblockplus.org/29459562/diff/29459563/lib/utils.js#newcod...
lib/utils.js:564: return new Intl.DateTimeFormat(Utils.appLocale, {
On 2017/06/08 08:13:01, Wladimir Palant wrote:
> Originally, I meant to pass undefined as first parameter here, which should
make
> the browser fall back to the default locale (supposedly, system's locale).
> However, this produced inconsistent results on my system which uses a mixture
of
> English and German locale settings (LANG=en_US.UTF-8, LC_TIME=de_DE.UTF-8).
> Firefox 53 used the German date format whereas Firefox 55 and Chrome chose the
> US format. So I thought that a date format matching our UI language might be
> more consistent. Not really sure about the expectations of somebody using an
> English-language browser however.

After looking a bit more into this, the inconsistency appears to be a bug that
affects Date.toLocaleString() as well:
https://bugzilla.mozilla.org/show_bug.cgi?id=1346674#c16. It's probably better
to use the system-wide date format so I switched back to undefined here.

[Thomas Greiner, 2017-06-16 11:02:11]

LGTM