Bug 169182 – Currency digits calculation in Intl.NumberFormat should call out to ICU

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Daniel Ehrenberg]

Intl.NumberFormat currently uses a fixed list of currencies which have a number of decimal digits different from 2. I wrote a patch to call out to ICU's function ucurr_getDefaultFractionDigits instead and will shortly upload it as an alternate solution. This is less code and may give an updated list of currencies in the future when ICU updates.

[Daniel Ehrenberg]

Created attachment 303458 [details]
Patch

[Daniel Ehrenberg]

Created attachment 303505 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 303505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303505&action=review

r=me

> Source/JavaScriptCore/ChangeLog:11
> +        (JSC::extractCurrencySortKey): Deleted.

If it involves some behavior change, can you add a test for that? (in JSTests/stress/)

[Daniel Ehrenberg]

This patch does not involve any behavior change.

[Yusuke Suzuki]

Comment on attachment 303505 [details]
Patch

OK. I've just set cq+. This patch will be landed through the commit queue.
You can set cq=? in addition to r=? to request reviewers / committers to post to cq when r+ is set :)

[WebKit Commit Bot]

Comment on attachment 303505 [details]
Patch

Clearing flags on attachment: 303505

Committed r213447: <http://trac.webkit.org/changeset/213447>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Ehrenberg]

Sorry, it turns out I was wrong when I thought this patch didn't change observable behavior. It actually introduces a spec violation, though it matches V8 behavior, and I suspect the right fix is to change the spec eventually. Details are in https://github.com/tc39/ecma402/issues/134 .

Either way, in the short term, maybe this patch should be reverted. Is there a particular procedure for making a revert, or should I just upload another patch that reverts this one?

[Darin Adler]

If you haven’t already, please just upload another patch that reverts this one.

[Darin Adler]

It’s often a mistake to use a library directly in WebKit; often the people changing those libraries don’t consider the effect on web technology when they change them. Because of this we often need a white list or other mechanism between WebKit and the underlying library.

(In fact, we likely need to do this for character encodings and ICU if we haven’t done it already.)

[Daniel Ehrenberg]

In this case, the reason why it's a mistake is very subtle. The library gives linguistically more accurate information than what WebKit uses otherwise. Updates to the library should be for the same goal as providing linguistically accurate information. WebKit makes heavy use of ICU in other places. Chromium has long called out to ICU for this information, and three major browsers use ICU for other parts of Intl.NumberFormat generally. Do you think that WebKit would benefit from reimplementing icu::DecimalFormat to reduce dependency on ICU in its ECMA 402 implementation?

Here, use of the library is disallowed because the specification of JavaScript requires a particular information source with more legalistic and less linguistically relevant data. I'm pursuing a change in the specification at  https://github.com/tc39/ecma402/issues/134 because I think it will give users better behavior.

Anyway, sorry for the churn here! I'll upload a revert patch soon.

[Daniel Ehrenberg]

Reopening to attach new patch.

[Daniel Ehrenberg]

Created attachment 304259 [details]
Patch

[Saam Barati]

Comment on attachment 304259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304259&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        Revert patch to call out to ICU for CurrencyDigits

Nit: please say which revision you're rolling out.

> Source/JavaScriptCore/ChangeLog:5
> +        The previous patch was currently technical invalid because ECMA 402

Style nit: we usually put this after the "reviewed by" line. 
Also, "currently techinical invalid" => "is technically invalid"?

[Daniel Ehrenberg]

Created attachment 304272 [details]
Patch

[Daniel Ehrenberg]

Thanks for the quick reviews. I fixed up the commit message in a new version of the patch.

[WebKit Commit Bot]

Comment on attachment 304272 [details]
Patch

Clearing flags on attachment: 304272

Committed r214020: <http://trac.webkit.org/changeset/214020>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.