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.
Created attachment 303458 [details] Patch
Created attachment 303505 [details] Patch
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/)
This patch does not involve any behavior change.
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 :)
Comment on attachment 303505 [details] Patch Clearing flags on attachment: 303505 Committed r213447: <http://trac.webkit.org/changeset/213447>
All reviewed patches have been landed. Closing bug.
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?
If you haven’t already, please just upload another patch that reverts this one.
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.)
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.
Reopening to attach new patch.
Created attachment 304259 [details] Patch
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"?
Created attachment 304272 [details] Patch
Thanks for the quick reviews. I fixed up the commit message in a new version of the patch.
Comment on attachment 304272 [details] Patch Clearing flags on attachment: 304272 Committed r214020: <http://trac.webkit.org/changeset/214020>