966559 - Internationalization API uses ICU C++ API for NumberingSystem

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect)

Description

[Norbert Lindenberg]

The implementation of the ECMAScript Internationalization uses the ICU C++ API to obtain information about the default numbering system for a locale. Using the C++ API is unsafe when using an ICU library provided by the operating system:
http://userguide.icu-project.org/design#TOC-ICU-Binary-Compatibility:-Using-ICU-as-an-Operating-System-Level-Library

The C++ API was used here because there was no equivalent C API at the time:
http://bugs.icu-project.org/trac/ticket/10039
The missing C API has since been added in ICU 52.

Comment 1

[Norbert Lindenberg]

Attachment: Use new C API instead of C++ API

This patch builds with the Internationalization API enabled or disabled, and passes the intl402 tests if build with the Internationalization API enabled.

However, the new C API in ICU is still marked as draft in the API documentation, so it may be to early to use it in production:
http://icu-project.org/apiref/icu4c/unumsys_8h.html

Comment 2

[Norbert Lindenberg]

Attachment: Use new C API instead of C++ API

Now with updated check_spidermonkey_style.py. Try run at
https://tbpl.mozilla.org/?tree=Try&rev=1e768a6157d7

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Updated partial patch

This patch would be complete, but on b2g where we use the system ICU in some configurations, the system ICU is "50.1+" per build/autoconf/icu.m4.  According to this bug, unumsys.h was added in 52.1, so landing this patch as-is would break b2g with system ICU.

Where's that leave this patch?  Pocketed, I guess, until system b2g is newer.  Or maybe some sort of adjustment can be made based on $ANDROID_VERSION.  I dunno.  Not important enough to investigate and possibly solve now.

Comment 5

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

Can we now land it?

Comment 6

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

Attachment: Bug 966559 - Use public C API for NumberingSystem.

Review commit: https://reviewboard.mozilla.org/r/89888/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89888/

Comment 7

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

Dusted off the patch.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8806460 [details]
Bug 966559 - Use public C API for NumberingSystem.

https://reviewboard.mozilla.org/r/89888/#review89442

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c28c8e058e17
Use public C API for NumberingSystem. r=Waldo

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c28c8e058e17