Closed Bug 866301 Opened 11 years ago Closed 9 years ago

Enable ECMAScript Internationalization API for Firefox OS

Categories

(Core :: JavaScript Engine, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- wontfix
firefox39 --- wontfix
firefox40 --- fixed
b2g-master --- fixed

People

(Reporter: mozillabugs, Assigned: m_kato)

References

(Blocks 3 open bugs, )

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(7 files, 5 obsolete files)

(deleted), patch
glandium
: feedback+
Details | Diff | Splinter Review
(deleted), patch
glandium
: review+
m_kato
: checkin+
Details | Diff | Splinter Review
(deleted), application/x-sharedlib
Details
(deleted), patch
glandium
: review+
m_kato
: checkin+
Details | Diff | Splinter Review
(deleted), text/x-log
Details
(deleted), patch
fabrice
: review+
Waldo
: review+
m_kato
: checkin+
Details | Diff | Splinter Review
(deleted), patch
glandium
: review+
m_kato
: checkin+
Details | Diff | Splinter Review
Enable the implementation of the ECMAScript Internationalization API, which has so far been landed disabled, for Firefox OS.

See bug 853301 for enabling the API on desktop.
Some people have suggested using the ICU that's part of the Android sources which in turn are part the B2G sources to implement the internationalization API. According to Chris Jones, that copy of ICU isn't actually used in B2G, and some OEMs have already removed it.
Blocks: 866344
Blocks: 839645
Blocks: 883474
Blocks: 883269
Blocks: 911870
Blocks: 916647
Blocks: 914589
Blocks: 914558
FTR, a partner needs it for a launch market of Firefox OS 1.2.
Jeff, any chance you'd be interested in working on that? Is that only blocked by bug 924839? Would it be hard to get it for fxos once that one is solved?


It's becoming an increasingly itching issue and developing dirty hacks that we'll want to eventually replace with ICU sounds like a bad investment.
Flags: needinfo?(jwalden+bmo)
It's blocked by that, but much more importantly it's blocked by getting ICU working in cross-compiling setups.  We don't handle that at all right now.  That sort of build-system hackery is not my strong suit, so I'm not really the best person to handle it, but I could probably fake it if absolutely necessary.
Flags: needinfo?(jwalden+bmo)
Depends on: 912371
To fix this, we require adding $(gonkdir)/abi/cpp/include to include path for RTTI support that is needed by ICU.  Also, we need link libgabi++ for RTTI, also.
Assignee: general → m_kato
Depends on: 924839
Blocks: 924851
Blocks: 724534
Blocks: 724535
Blocks: 724538
Blocks: 871846
Blocks: 933631
Blocks: 820261
Hi all, what's the status of this bug? Any chance to have it fixed by 2.0?
(In reply to Rimas Kudelis from comment #6)
> Hi all, what's the status of this bug? Any chance to have it fixed by 2.0?

This bug will be fixed by bug 864843.
Blocks: 556237
gonk has libgapi++ as system.  So when ICU can use it for RTTI suport, it can resolve RTTI issue even if gabi++ isn't imported.

patch will be coming this week.
Depends on: 1051669
Hi Makoto -- have you been able to look into this bug lately?
Attached patch Part 1. Add build config for ICU on gonk (obsolete) (deleted) — Splinter Review
Attached patch Part 2. Turn on ICU on all B2G build (obsolete) (deleted) — Splinter Review
Comment on attachment 8480492 [details] [diff] [review]
Part 1. Add build config for ICU on gonk

Gonk already has gabi++ into source tree.

Also, although INTL API check is tail of js/src/configure.in now, should I move it to before AC_SUBST(OS_LIBS) or LIBS="xxxx -lgabi++"?
Attachment #8480492 - Flags: review?(mh+mozilla)
Whiteboard: [DocArea=JS]
Comment on attachment 8480493 [details] [diff] [review]
Part 2. Turn on ICU on all B2G build

Should I add additional reviewer of B2G team?
Attachment #8480493 - Flags: review?(jwalden+bmo)
Comment on attachment 8480493 [details] [diff] [review]
Part 2. Turn on ICU on all B2G build

Fabrice is a co-owner of the b2g runtime, so he probably makes sense (https://wiki.mozilla.org/Modules/FirefoxOS).  I suspect the biggest thing is seeing what this does to the gecko footprint on devices...
Attachment #8480493 - Flags: review?(fabrice)
Comment on attachment 8480493 [details] [diff] [review]
Part 2. Turn on ICU on all B2G build

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

My understanding is multiple ICUs on b2g isn't going to fly.  Or at least it won't fly on the lowest-end devices with the least memory.  The space hit from shipping *all* the locale data may be problematic, too.

The sense I've gotten from speaking to various people is that until we can rip out the ICU embedded into libstagefright, we probably won't be able to ship our own ICU relatively simply embedded into Gecko, this way.  Instead we'll have to use the ICU in libstagefright.  I'm told we've already ripped out pretty much all the locale data from that ICU, so we'll have to package up our own locale data to drop onto the system, then the process of initializing ICU will have to be instructed to use those data files.

Unfortunately it's apparently non-trivial ripping out ICU from libstagefright, because it's not something we fully control -- we have to get partners to rip it out, everywhere, in addition to our ripping it out of our stuff.  (libstagefright uses ICU negligibly, so the size of the actual in-code dependency is basically zero.)  So while I expect it'll be great to use an ICU embedded in Gecko eventually, in the short run we'll likely need to figure out how to use libstagefright's copy, and we'll need to somehow decide what locale data to ship.  All the data for the locations we ship to, for sure, plus some set of well-used-enough languages or something.  I dunno, that's probably secondary after getting things working against libstagefright's ICU.

But this isn't a truly authoritative pronouncement; I'll let fabrice make that.  :-)
Attachment #8480493 - Flags: review?(jwalden+bmo)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16)
> Comment on attachment 8480493 [details] [diff] [review]
> Part 2. Turn on ICU on all B2G build
> 
> Review of attachment 8480493 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> My understanding is multiple ICUs on b2g isn't going to fly.  Or at least it
> won't fly on the lowest-end devices with the least memory.  The space hit
> from shipping *all* the locale data may be problematic, too.

ICU on GONK tree uses older version.  Gecko tree is 52.1, but Gonk-JB is still 50.1 now.
You know, Gecko tree needed the following patches when using 50.1 at least.

http://bugs.icu-project.org/trac/ticket/10045

If ICU on Gonk tree *always* keeps same version of Gecko tree, we only add another detection way without pkg_config.
If system is JB or later, it works this.  But system is ICS, ICU is 4x, so this option isn't supported.
How bad are the ICU 4x deficiencies?  Are they things we could hack around with enough effort, perhaps?  Or is it something horribly systemic that would be impossible to work around (like conceivably [although even this might be workaroundable, with pain] ICU not supporting BCP 47-style language tags in 4x)?
Comment on attachment 8480492 [details] [diff] [review]
Part 1. Add build config for ICU on gonk

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

::: toolkit/library/moz.build
@@ +77,5 @@
> +    if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':
> +        OS_LIBS += [
> +            'gabi++',
> +        ]
> +

You should just add this OS_LIBS to config/external/icu/moz.build, and that should work without touching toolkit/library/moz.build and js/src/configure.in.
Attachment #8480492 - Flags: review?(mh+mozilla) → feedback+
Wow, all the issues this depends on appear fixed! That's good news for the 18 (!) issues dependent on this one. Any prognosis?
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #19)
> How bad are the ICU 4x deficiencies?  Are they things we could hack around
> with enough effort, perhaps?  Or is it something horribly systemic that
> would be impossible to work around (like conceivably [although even this
> might be workaroundable, with pain] ICU not supporting BCP 47-style language
> tags in 4x)?

My concern is the compatibility of v4x.1 (ICS based) vs 51.1 (JB/KK based) vs 53.1 (L based. but not now).  
Outside of JS, some modules such as IndexedDB wants to use ICU for comparing etc.

If we shouldn't consider it, we should use system's ICU for footpirint.  But I think that we may fork it into gonk's source tree to upgrade 52.1 if possible.
And at least, due to bug such as http://bugs.icu-project.org/trac/ticket/10045, JS engine isn't same behavior if based OS is different.  If we can ignore it like this issue, we should remove version check.
UNUM_CURRENCY_ISO, UNUM_CURRENCY_PLURAL and UDAT_PATTERN aren't supported on ICS's ICU (version 4x).  So we requires 50.1 or later to build Intl API code.

https://tbpl.mozilla.org/php/getParsedLog.php?id=48858247&tree=Try&full=1
It seems we're only supporting KK and above now, at least for master? How do things look under that assumption?
What is the status of that? We could clean up a lot of code in Gaia and provide better APIs during the pre-3.0 cleanup if we got this.
Flags: needinfo?(m_kato)
(In reply to Zibi Braniecki [:gandalf] from comment #26)
> What is the status of that? We could clean up a lot of code in Gaia and
> provide better APIs during the pre-3.0 cleanup if we got this.

If minimal requirement of Firefox OS kernel is JB or later, it can be ignore footprint concern etc such as comment #15 and comment #16.

please make decision of update system's ICU as Gonk or we can use in-tree ICU for this issue (OS has 2 ICU so and it is bad for footprint).
Flags: needinfo?(m_kato) → needinfo?(fabrice)
I would rather use the in-tree ICU if possible, but unfortunately we still use ICS for emulator tests.
Can we use the in-tree for gonk >= JB and our own on ICS (we don't really have footprint issues on emulators).
Flags: needinfo?(fabrice)
FWIW, I haven't actually managed to build --with-system-icu on any version older than ICU 52.1. But after updating external/icu4c/ to ICU 52.1 it builds on the flame-kk.
Comment on attachment 8480493 [details] [diff] [review]
Part 2. Turn on ICU on all B2G build

outdated
Attachment #8480493 - Flags: review?(fabrice)
Attachment #8484073 - Attachment is obsolete: true
Attachment #8480493 - Attachment is obsolete: true
Attached patch Part 2. Use system ICU for gonk-JB or later (obsolete) (deleted) — Splinter Review
Attachment #8551651 - Attachment description: Bug 866301 - Part 1. Use in-tree ICU for gonk → Part 1. Use in-tree ICU for gonk
Attached patch Part 3. Turn on Intl API for B2G (obsolete) (deleted) — Splinter Review
Attachment #8551651 - Flags: review?(mh+mozilla)
Comment on attachment 8551652 [details] [diff] [review]
Part 2. Use system ICU for gonk-JB or later

Per comment #28.

If gonk is JB or later, we should use gonk's ICU instead of in-tree ICU.
Also, js/src/configure.in has no ANDRIOD_VERSION define, so I add it.
Attachment #8551652 - Flags: review?(mh+mozilla)
Comment on attachment 8551652 [details] [diff] [review]
Part 2. Use system ICU for gonk-JB or later

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

::: js/src/configure.in
@@ +193,5 @@
> +    dnl Default to ICS
> +    ANDROID_VERSION=15
> +    if test -n "${PLATFORM_SDK_VERSION}"; then
> +        ANDROID_VERSION="${PLATFORM_SDK_VERSION}"
> +    fi

Shouldn't this change be applied to /configure.in as well?
(In reply to Reuben Morais [:reuben] from comment #35)
> Comment on attachment 8551652 [details] [diff] [review]
> Part 2. Use system ICU for gonk-JB or later
> 
> Review of attachment 8551652 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/configure.in
> @@ +193,5 @@
> > +    dnl Default to ICS
> > +    ANDROID_VERSION=15
> > +    if test -n "${PLATFORM_SDK_VERSION}"; then
> > +        ANDROID_VERSION="${PLATFORM_SDK_VERSION}"
> > +    fi
> 
> Shouldn't this change be applied to /configure.in as well?

/configure.in has this code, but /js/src/configure.in doesn't have this.
Attached patch Add unorm.h to system-headers (deleted) — Splinter Review
This is used in a bunch of places, without adding it to system-headers we fail to link because of undefined references to the stuff defined in that header.
Attachment #8553905 - Flags: review?(mh+mozilla)
Attachment #8480492 - Attachment is obsolete: true
Comment on attachment 8551651 [details] [diff] [review]
Part 1. Use in-tree ICU for gonk

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

::: build/autoconf/icu.m4
@@ +94,5 @@
>                  if test -n "$MOZ_DEBUG" -a -z "$MOZ_NO_DEBUG_RTL"; then
>                      MOZ_ICU_DBG_SUFFIX=d
>                  fi
>                  ;;
> +            Darwin|Linux|DragonFly|FreeBSD|NetBSD|OpenBSD|GNU/kFreeBSD|Android)

Here's something I didn't catch last time: this should still fal through the AC_MSG_ERROR below when building for non-gonk Android.

::: config/external/icu/moz.build
@@ +18,5 @@
>          )]
> +
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':
> +    OS_LIBS += [
> +        'gabi++',

Where does the gabi++ library come from? My only reference is the android ndk, and there, it's gabi++_static or gabi++_shared. Can you attach that library as to assess how things will look like to the linker?
Attachment #8551651 - Flags: review?(mh+mozilla) → feedback+
Comment on attachment 8551652 [details] [diff] [review]
Part 2. Use system ICU for gonk-JB or later

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

::: build/autoconf/icu.m4
@@ +21,5 @@
>      MOZ_SHARED_ICU=1
> +elif test -n "$gonkdir" -a "$ANDROID_VERSION" -ge 17; then
> +    dnl Use system's ICU since version is 50.1+.
> +    MOZ_ICU_CFLAGS="-I$gonkdir/external/icu4c/common -I$gonkdir/external/icu4c/i18n"
> +    MOZ_ICU_LIBS='-licui18n -licuuc'

-licudata should be in there too, for consistency.
Attachment #8551652 - Flags: review?(mh+mozilla) → review+
Attachment #8553905 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #38)
> Comment on attachment 8551651 [details] [diff] [review]
> Part 1. Use in-tree ICU for gonk
> 
> Review of attachment 8551651 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/autoconf/icu.m4
> @@ +94,5 @@
> >                  if test -n "$MOZ_DEBUG" -a -z "$MOZ_NO_DEBUG_RTL"; then
> >                      MOZ_ICU_DBG_SUFFIX=d
> >                  fi
> >                  ;;
> > +            Darwin|Linux|DragonFly|FreeBSD|NetBSD|OpenBSD|GNU/kFreeBSD|Android)
> 
> Here's something I didn't catch last time: this should still fal through the
> AC_MSG_ERROR below when building for non-gonk Android.

OK.  I will add it
 
> ::: config/external/icu/moz.build
> @@ +18,5 @@
> >          )]
> > +
> > +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':
> > +    OS_LIBS += [
> > +        'gabi++',
> 
> Where does the gabi++ library come from? My only reference is the android
> ndk, and there, it's gabi++_static or gabi++_shared. Can you attach that
> library as to assess how things will look like to the linker?

libgai++.so is generated into <B2G>/out/target/product/<flame>/obj/lib/libgabi++.so by <B2G>/abi/cpp/src.  It is from gonk.
(In reply to Mike Hommey [:glandium] from comment #39)
> Comment on attachment 8551652 [details] [diff] [review]
> Part 2. Use system ICU for gonk-JB or later
> 
> Review of attachment 8551652 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/autoconf/icu.m4
> @@ +21,5 @@
> >      MOZ_SHARED_ICU=1
> > +elif test -n "$gonkdir" -a "$ANDROID_VERSION" -ge 17; then
> > +    dnl Use system's ICU since version is 50.1+.
> > +    MOZ_ICU_CFLAGS="-I$gonkdir/external/icu4c/common -I$gonkdir/external/icu4c/i18n"
> > +    MOZ_ICU_LIBS='-licui18n -licuuc'
> 
> -licudata should be in there too, for consistency.

B2G has no libicudata.so.

Gonk's ICU is same as AOSP.  So it doesn't generate libicudata.so.  AOSP souce already has icudat<version>-all.dat file by pre-generataed by Android team.

So icudata is data file as icudt<version number>.dat into /usr/icu/, not shared library.
(In reply to Makoto Kato (:m_kato) from comment #40)
> libgai++.so is generated into
> <B2G>/out/target/product/<flame>/obj/lib/libgabi++.so by <B2G>/abi/cpp/src. 
> It is from gonk.

Can you attach yours?

(In reply to Makoto Kato (:m_kato) from comment #41)
> (In reply to Mike Hommey [:glandium] from comment #39)
> > Comment on attachment 8551652 [details] [diff] [review]
> > Part 2. Use system ICU for gonk-JB or later
> > 
> > Review of attachment 8551652 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: build/autoconf/icu.m4
> > @@ +21,5 @@
> > >      MOZ_SHARED_ICU=1
> > > +elif test -n "$gonkdir" -a "$ANDROID_VERSION" -ge 17; then
> > > +    dnl Use system's ICU since version is 50.1+.
> > > +    MOZ_ICU_CFLAGS="-I$gonkdir/external/icu4c/common -I$gonkdir/external/icu4c/i18n"
> > > +    MOZ_ICU_LIBS='-licui18n -licuuc'
> > 
> > -licudata should be in there too, for consistency.
> 
> B2G has no libicudata.so.
> 
> Gonk's ICU is same as AOSP.  So it doesn't generate libicudata.so.  AOSP
> souce already has icudat<version>-all.dat file by pre-generataed by Android
> team.
> 
> So icudata is data file as icudt<version number>.dat into /usr/icu/, not
> shared library.

Please add a comment to that effect.
Attached file libgabi++.so on Flame-KK (deleted) —
(In reply to Makoto Kato (:m_kato) from comment #43)
> Created attachment 8555071 [details]
> libgabi++.so on Flame-KK

Ok, that looks fine. Interesting, it has references to malloc and free that aren't used.
Comment on attachment 8569755 [details] [diff] [review]
Part 1. Use in-tree ICU for gonk v2

I also modify to add OS_LIBS for in-tree ICU building only.
Attachment #8569755 - Flags: review?(mh+mozilla)
Attachment #8569755 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8569755 [details] [diff] [review]
Part 1. Use in-tree ICU for gonk v2

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

::: config/external/icu/moz.build
@@ +18,5 @@
>          )]
> +
> +    if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':
> +        OS_LIBS += [
> +            'gabi++',

Actually, let me retract a bit on this. Why does e.g. libxul need to link again gabi++ when linking icu?
Attachment #8569755 - Flags: review+
(In reply to Mike Hommey [:glandium] from comment #47)
> Comment on attachment 8569755 [details] [diff] [review]
> Part 1. Use in-tree ICU for gonk v2
> 
> Review of attachment 8569755 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/external/icu/moz.build
> @@ +18,5 @@
> >          )]
> > +
> > +    if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':
> > +        OS_LIBS += [
> > +            'gabi++',
> 
> Actually, let me retract a bit on this. Why does e.g. libxul need to link
> again gabi++ when linking icu?

Now, we link libicu as static library.  So, no way to add additional library such as libgabi++.(so|a) to AR in libicu.

ex. http://mxr.mozilla.org/mozilla-central/source/intl/icu/source/i18n/Makefile.in?mark=169-173#169

169 ifneq ($(ENABLE_STATIC),)
170 $(TARGET): $(STATIC_OBJECTS)
171 	$(AR) $(ARFLAGS) $(AR_OUTOPT)$@ $^
172 	$(RANLIB) $@
173 endif

If we built libicu as shared library, we don't need link gabi++ on linking XUL.
Any prognosis on this? Issue 922963, which is very important for RTL support, depends on this.
(In reply to Mike Hommey [:glandium] from comment #47)
> Actually, let me retract a bit on this. Why does e.g. libxul need to link
> again gabi++ when linking icu?

libgabi++ is built as shared library only.  So although I think "ar" cannot link shared library to static library, is there a way to link shared library of libgabi++ to static library of libicu??

Or should I add libgabi++ source code to our tree for gonk-ICS, then built it as static??
Flags: needinfo?(mh+mozilla)
Let me rephrase the question: what symbols does icu need from libgabi++?
Flags: needinfo?(m_kato)
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #51)
> Let me rephrase the question: what symbols does icu need from libgabi++?

ICU requreis RTTI, so all symbols seems be RTTI.

__dynamic_cast
std::type_info::operator!=(std::type_info const&) const
std::type_info::operator!=(std::type_info const&) const
std::type_info::operator==(std::type_info const&) const
vtable for __cxxabiv1::__class_type_info
vtable for __cxxabiv1::__si_class_type_info
vtable for __cxxabiv1::__vmi_class_type_info
Flags: needinfo?(m_kato)
Attached file error.log when don't link libgabi++ (deleted) —
Flags: needinfo?(mh+mozilla)
Attachment #8569755 - Flags: review+
Flags: needinfo?(mh+mozilla)
Keywords: leave-open
Attachment #8569755 - Flags: checkin+
Attachment #8553905 - Flags: checkin+
ICU path is changed from lolipop gonk.  I need modify system ICU detection.
Attachment #8551653 - Attachment is obsolete: true
Attachment #8551652 - Attachment is obsolete: true
Attachment #8595824 - Attachment description: Use system ICU for gonk-JB or later → Part 3. Use system ICU for gonk-JB or later
Comment on attachment 8595824 [details] [diff] [review]
Part 3. Use system ICU for gonk-JB or later

Updated old fix.

ICU path of gonk-L is changed to external/icu/icu4c/source.  gonk-JB and gonk-KK are still old path (external/icu4c).
Attachment #8595824 - Flags: review?(mh+mozilla)
Attachment #8595824 - Flags: review?(mh+mozilla) → review+
Attachment #8595824 - Flags: checkin+
Comment on attachment 8595246 [details] [diff] [review]
Part 4. Turn on Intl API for B2G

Turn on ECMA-402 support for B2G as default and fix test for B2G
Attachment #8595246 - Flags: review?(jwalden+bmo)
Attachment #8595246 - Flags: review?(fabrice)
Comment on attachment 8595246 [details] [diff] [review]
Part 4. Turn on Intl API for B2G

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

lgtm
Attachment #8595246 - Flags: review?(fabrice) → review+
Comment on attachment 8595246 [details] [diff] [review]
Part 4. Turn on Intl API for B2G

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

Facially fine, but it's not my call as to *whether* to do this or not.  I'll assume Fabrice's say-so counts.
Attachment #8595246 - Flags: review?(jwalden+bmo) → review+
Attachment #8595246 - Flags: checkin+
All patches were landed and test is passed.  Closing this.
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1167052
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: