Enable the implementation of the ECMAScript Internationalization API, which has so far been landed disabled, for Firefox on Android. See bug 853301 for enabling the API on desktop.

Do we expect this to result in a ~3Mb in binary size for Fennec as well? Either way, this winds up being a product (Karen) call.

The size increase depends a lot on the compression used; if Fennec binaries are compressed the same way as other Linux builds, then yes, it would be about 3MB if no other changes are made. I have a document that discusses some implementation options for the internationalization API: http://lindenbergsoftware.com/mozilla/implementation.html The ideas for further reducing the size increase are discussed in the "Possible additional steps" section.

need info to Karen for the product decision on increasing the size by 3Mb

I am, indeed, concerned about the 3MB increase from a mobile standpoint. Norbert, you discuss about some different options - how viable are they and how effective could they be? For RAM, could we have some spot testing done to ensure we don't break our lowest-speced RAM phones (down to 384MB)?

Brad, can we use the system ICU? /be

(In reply to Karen Rudnitski from comment #4) > I am, indeed, concerned about the 3MB increase from a mobile standpoint. > Norbert, you discuss about some different options - how viable are they and > how effective could they be? For RAM, could we have some spot testing done > to ensure we don't break our lowest-speced RAM phones (down to 384MB)? My document lists the options roughly in order of viability as I see it - i.e., removing locales is quite viable if you as the product manager agree; while I consider developing special-purpose compression or reimplementing the relevant parts of ICU in JavaScript is research projects with uncertain outcomes. I should mention that my contract with Mozilla has ended, and my contract manager and I have been assuming that engineering work specific to Firefox on Android would have to be done by the team working on that product.

(In reply to Brendan Eich [:brendan] from comment #5) > Brad, can we use the system ICU? No, the ICU built into Android is only available to system applications - even Chrome on Android brings along its own copy. Mozilla could ask Google to make ICU available as part of the NDK; this would allow the use of the system ICU at least on future versions of Android.

(In reply to Norbert Lindenberg from comment #7) > (In reply to Brendan Eich [:brendan] from comment #5) > > Brad, can we use the system ICU? > > No, the ICU built into Android is only available to system applications - > even Chrome on Android brings along its own copy. Mozilla could ask Google > to make ICU available as part of the NDK; this would allow the use of the > system ICU at least on future versions of Android. I do see that the ICU data file might be accessible on Android, even though we would need to ship the code itself. The data file on my Gingerbread phone is at /system/usr/icu: -rw-r--r-- root root 6013648 2012-02-13 22:10 icudt44l.dat Also, if we want to tweak the data file, as Norbert suggests, we could use this online tool to remove locales we don't want/need to support: http://apps.icu-project.org/datacustom/ICUData46.html

Norbert, can you prepare a patch to use the system ICU?

Norbert was contracting, and his contract's finished, so I'm on point for this. I don't expect I'll get to this before I finish up bug 853301, though. (Particularly as dealing with this probably requires dealing with the cross-compilation issue there anyway.)

Although Chrome uses ICU, it depends on android source to use gabi++ (it includes <androud source>/abi/cpp/include). But we use stlport that doesn't have RTTI support and typeid. After fixing bug 912371 (cross compile support of ICU) and bug 911893 (--enable-android-libstdcxx fix), we will be able to turn on ICU on Android if using --enable-android-libstdcxx. And, current Android/B2G uses ICU 4x, but ECMA-402 support in our code requires ICU 50.1. So we cannot use system ICU. And even if Android uses 50.1 at feature, android build option of system ICU uses --with-library-suffix, so we cannot link it to Fennec because all functions have version suffix. (ex. ubrk_oepn_48)

To build ICU for android, it requires http://bugs.icu-project.org/trac/changeset/34152 (fixed in 52.1) at least.

WIP. It requires ICU 52.1 or r34152 diff of icu to use --disable-tools

Comment on attachment 8340900 [details] [diff] [review] Import changeset r34152 patch of ICU to m-c If bug 924839 (update ICU 52.1) is fixed soon, this is unnecessary.

Comment on attachment 8340901 [details] [diff] [review] Apply changeset r34152 If bug 924839 (update ICU 52.1) is fixed soon, this is unnecessary.

RTTI support for Android requires gabi++. And ICU 52.1 has --disable-tools for Android target　(http://bugs.icu-project.org/trac/changeset/34152).

Makoto - What APK size changes to you see from your builds?

(In reply to Mark Finkle (:mfinkle) from comment #22) > Makoto - What APK size changes to you see from your builds? 3.6MB. Before ====== -rw-r--r-- 1 makoto users 26355419 Dec 11 12:54 fennec-29.0a1.en-US.android-arm.apk -rw-r--r-- 1 makoto users 14046746 Dec 11 12:54 libxul.so After ===== -rw-r--r-- 1 makoto users 29919067 Dec 11 12:32 fennec-29.0a1.en-US.android-arm.apk -rw-r--r-- 1 makoto users 17610376 Dec 11 12:32 libxul.so

Comment on attachment 8344488 [details] [diff] [review] Build ICU for Android Review of attachment 8344488 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/tests/mochitest/general/test_interfaces.html @@ +60,5 @@ > "Int16Array", > "Int32Array", > "Int8Array", > "InternalError", > + {name: "Intl", b2g: false}, Note that desktop or mobile/android is not "not b2g" (think of e.g. xulrunner, seamonkey, thunderbird...) ::: js/src/configure.in @@ +4238,5 @@ > if test -z "$MOZ_SHARED_ICU"; then > MOZ_ICU_LIBS='$(call EXPAND_LIBNAME_PATH,$(ICU_LIB_NAMES),$(DEPTH)/intl/icu/target/lib)' > + if test "$OS_TARGET" = "Android" -a -z "$gonkdir"; then > + # require linking libgabi++_static for RTTI support > + MOZ_ICU_LIBS="$MOZ_ICU_LIBS \$(call EXPAND_LIBNAME_PATH,gabi++_static,\$(ANDROID_NDK)/sources/cxx-stl/gabi++/libs/\$(ANDROID_CPU_ARCH))" We had problems in the past when linking the stlport static libraries provided in the NDK (because of flags mismatch between how the static libs were built and how our binaries are built), and we had to (re)build it ourselves. We'd need the same here.

Comment on attachment 8340900 [details] [diff] [review] Import changeset r34152 patch of ICU to m-c Review of attachment 8340900 [details] [diff] [review]: ----------------------------------------------------------------- I suppose I'm competent to review these as an ICU importing change. :-) This is basically fine on that front. But the contents of the patch are mostly opaque to me, unless I spend a bunch of time reading/investigating (and even then, build stuffs aren't my strong suit, so probably best I don't do it). Given that you say this is fixed in ICU 52.1, I'm going to assume we want to wait on bug 924839 at this point here. If that for some reason becomes not an issue, I think I'd prefer to see this patch and the other one combined into one, to make the changes to our imported ICU *and* add the local patch all in one fell swoop.

(In reply to Mike Hommey [:glandium] from comment #24) > Comment on attachment 8344488 [details] [diff] [review] > Build ICU for Android > > Review of attachment 8344488 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/tests/mochitest/general/test_interfaces.html > @@ +60,5 @@ > > "Int16Array", > > "Int32Array", > > "Int8Array", > > "InternalError", > > + {name: "Intl", b2g: false}, > > Note that desktop or mobile/android is not "not b2g" (think of e.g. > xulrunner, seamonkey, thunderbird...) The current "desktop: true" doesn't exclude non-Firefox apps anyway.

It can build for gonk too if importing libgabi++ from android source code.

(In reply to Mike Hommey [:glandium] from comment #24) > Comment on attachment 8344488 [details] [diff] [review] > Build ICU for Android > > Review of attachment 8344488 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/tests/mochitest/general/test_interfaces.html > @@ +60,5 @@ > > "Int16Array", > > "Int32Array", > > "Int8Array", > > "InternalError", > > + {name: "Intl", b2g: false}, > > Note that desktop or mobile/android is not "not b2g" (think of e.g. > xulrunner, seamonkey, thunderbird...) Yes. But we don't run mochitest on these products.

I, for one, run mochitests on xulrunner.

(In reply to Mike Hommey [:glandium] from comment #29) > I, for one, run mochitests on xulrunner. It should be already broken as I said in comment #26.

Why Intl is disabled on non-Firefox products in the first place? The Gecko capability should be aligned across all products.

How can bug 950370 be missed while running mochitests on xulrunner?

(In reply to Masatoshi Kimura [:emk] from comment #31) > Why Intl is disabled on non-Firefox products in the first place? The Gecko > capability should be aligned across all products. See bug 853301 and old post at mozilla.dev.platform discussion. (subject: "Integrating ICU into Mozilla build" and "Cost of ICU data"). But, Ehsan wants to use ICU for bug 871846, so it will be turned on all platform / products by that bug.

(In reply to Mike Hommey [:glandium] from comment #29) > I, for one, run mochitests on xulrunner. Where could I check log for mochitest on xulrunner by tbpl/tinderbox? There is no log on ftp server. Also, since intl-api is turned off xulrunner build, I think that test_interfaces.html is always failure.

(In reply to Makoto Kato (:m_kato) from comment #34) > (In reply to Mike Hommey [:glandium] from comment #29) > > I, for one, run mochitests on xulrunner. > > Where could I check log for mochitest on xulrunner by tbpl/tinderbox? There > is no log on ftp server. Because there is no such thing. You need to run them manually.

Comment on attachment 8348513 [details] [diff] [review] Part 1. Import libgabi++ from https://android.googlesource.com/platform/abi/cpp/ Import gabi++ to use RTTI support for Android and Gonk

Comment on attachment 8348515 [details] [diff] [review] Part 2. Build libgabi++ for ICU build gabi++ to use RTTI support for Android and Gonk

Comment on attachment 8348517 [details] [diff] [review] Part 3. ICU build for Android/Gonk Build ICU using gabi++ if needed. But intl-api is still turned off as default.

Comment on attachment 8348513 [details] [diff] [review] Part 1. Import libgabi++ from https://android.googlesource.com/platform/abi/cpp/ Review of attachment 8348513 [details] [diff] [review]: ----------------------------------------------------------------- Please put it under build/, not js/src/build. Also put a file in the directory indicating where this was picked from. Bonus points if you make that a script that could be used for future imports. See other such scripts we have throughout the tree.

Comment on attachment 8348515 [details] [diff] [review] Part 2. Build libgabi++ for ICU Review of attachment 8348515 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/build/gabi++/Makefile.in @@ +1,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this file, > +# You can obtain one at http://mozilla.org/MPL/2.0/. > + > +MODULES = gabi++ I guess you got that from stlport, but that's actually leftover due to a typo. IOW, it's useless. @@ +11,5 @@ > + > +include $(topsrcdir)/config/rules.mk > + > +# RTTI and C++ exception support are required > +OS_CXXFLAGS := $(filter-out -fno-rtti -fno-exceptions,$(OS_CXXFLAGS)) -frtti -fexceptions You should be able to do OS_CXXFLAGS += -frtti -fexceptions ::: js/src/build/gabi++/README.mozilla @@ +1,1 @@ > +This copy of libgabi++ is from Android 4.4 source code on https://android.googlesource.com/platform/abi/cpp/. Ah, this should be in part 1. ::: js/src/moz.build @@ +3,5 @@ > # This Source Code Form is subject to the terms of the Mozilla Public > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +DIRS += ['config', 'build'] You'll need to make this part of the toplevel build, not js. I don't think it really matters because a) i doubt people would care about not being able to build js standalone for android with icu b) the separation between toplevel and js is going to go away in a matter of weeks.

Comment on attachment 8348517 [details] [diff] [review] Part 3. ICU build for Android/Gonk Review of attachment 8348517 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +8789,5 @@ > Darwin) > ICU_LIB_NAMES="icui18n icuuc icudata" > MOZ_ICU_LIBS='$(foreach lib,$(ICU_LIB_NAMES),$(DEPTH)/js/src/intl/icu/target/lib/$(DLL_PREFIX)$(lib).$(MOZ_ICU_VERSION)$(DLL_SUFFIX))' > ;; > + Linux|DragonFly|FreeBSD|NetBSD|OpenBSD|Android) Might as well change the test to be on OS_ARCH instead of OS_TARGET. ::: js/src/Makefile.in @@ +223,5 @@ > # - Force ICU to use the standard suffix for object files because expandlibs > # will discard all files with a non-standard suffix (bug 857450). > # - Options for genrb: -k strict parsing; -R omit collation tailoring rules. > buildicu: > + $(call SUBMAKE,,build) what is this supposed to achieve? @@ +232,5 @@ > +$(ICU_MAKE) -j1 -C intl/icu/target STATIC_O=$(OBJ_SUFFIX) GENRBOPTS='-k -R' > $(ICU_LIB_RENAME) > > distclean clean:: > + $(call SUBMAKE,$@,build) what is this supposed to achieve? ::: js/src/configure.in @@ +4233,5 @@ > if test -z "$MOZ_SHARED_ICU"; then > MOZ_ICU_LIBS='$(call EXPAND_LIBNAME_PATH,$(ICU_LIB_NAMES),$(DEPTH)/intl/icu/target/lib)' > + if test "$OS_TARGET" = "Android" -a -z "$MOZ_ANDROID_LIBSTDCXX"; then > + # require linking libgabi++_static for RTTI support > + MOZ_ICU_LIBS="$MOZ_ICU_LIBS \$(call EXPAND_LIBNAME_PATH,gabi++_static,$_objdir/build/gabi++)" You're not handling the MOZ_SHARED_ICU case.

When we use shared icu on Android, we need add these files to installer. Also, same issue on b2g desktop build. (currently, we cannot turn on icu on b2g yet)

import into build/ intead of js

Build libgabi++ when turn on intl-api.

To build rtti support (it requires ICU), gabi++ is required. gabi++ is Apache 2.0 license (see https://android.googlesource.com/platform/abi/cpp/)

Comment on attachment 8358247 [details] [diff] [review] Part 3. Support ICU building for Android/Gonk OS_ARCH isn't normalized for BSD. And if supporting shared-icu for Android target, we need build gabi++ into configure because ICU's configure checks.

(In reply to Makoto Kato (:m_kato) from comment #49) > Created attachment 8358214 [details] [diff] [review] > Part 4. Update about:license for gabi++ > > To build rtti support (it requires ICU), gabi++ is required. gabi++ is > Apache 2.0 license (see https://android.googlesource.com/platform/abi/cpp/) You don't mean Apache 2.0, do you? You mean the Android BSD Licence. That's the license on the files you link to, and that's the license to which your patch adds a directory annotation. Gerv

Comment on attachment 8358247 [details] [diff] [review] Part 3. Support ICU building for Android/Gonk Review of attachment 8358247 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +8818,5 @@ > Darwin) > ICU_LIB_NAMES="icui18n icuuc icudata" > MOZ_ICU_LIBS='$(foreach lib,$(ICU_LIB_NAMES),$(DEPTH)/js/src/intl/icu/target/lib/$(DLL_PREFIX)$(lib).$(MOZ_ICU_VERSION)$(DLL_SUFFIX))' > ;; > + Linux|*bsd*|dragonfly*) OS_ARCH has the same values as OS_TARGET for dragonfly and the BSDs. ::: js/src/configure.in @@ +4419,5 @@ > + ICU_CXXFLAGS="-I$_topsrcdir/../../build/gabi++/include $ICU_CXXFLAGS" > + > + if test -n "$MOZ_SHARED_ICU"; then > + # need libgabi++_static for ICU's configure > + $GMAKE -C $_objdir/../../build/gabi++ Don't build from configure. Also, since the landing of bug 950298, those paths don't work :)

(In reply to Mike Hommey [:glandium] from comment #53) > Comment on attachment 8358247 [details] [diff] [review] > Part 3. Support ICU building for Android/Gonk > > Review of attachment 8358247 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: configure.in > @@ +8818,5 @@ > > Darwin) > > ICU_LIB_NAMES="icui18n icuuc icudata" > > MOZ_ICU_LIBS='$(foreach lib,$(ICU_LIB_NAMES),$(DEPTH)/js/src/intl/icu/target/lib/$(DLL_PREFIX)$(lib).$(MOZ_ICU_VERSION)$(DLL_SUFFIX))' > > ;; > > + Linux|*bsd*|dragonfly*) > > OS_ARCH has the same values as OS_TARGET for dragonfly and the BSDs. When cross compiling, OS_ARCH=`echo $target_os | sed -e 's|/|_|g'`. I should file a bug for normalized.

Comment on attachment 8358214 [details] [diff] [review] Part 4. Update about:license for gabi++ Please clarify issues in comment 52, and re-request review. Gerv

When building ICU as shared on Android/Gonk, static version of libgabi++ must be built. So we should mark as static=True on moz.build.

Comment on attachment 8375219 [details] [diff] [review] Part 2.1. Build libgabi++ as static before building icu sorry. it needs rebase

Comment on attachment 8375384 [details] [diff] [review] Part 2.1. Build libgabi++ as static before building ICU When building ICU as shared on Android/Gonk, static version of libgabi++ must be required before it. So we should mark as static=True on moz.build.

Comment on attachment 8375389 [details] [diff] [review] Part 3. Support ICU building for Android/Gonk I cannot pass -lgabi++_static to LDFLAGS of ICU's configure. Because ICU's configure checks parameter and libgabi++_static isn't built yet at this time. So I override LD_SOOPTIONS for shared.

Comment on attachment 8375384 [details] [diff] [review] Part 2.1. Build libgabi++ as static before building ICU Review of attachment 8375384 [details] [diff] [review]: ----------------------------------------------------------------- ::: moz.build @@ +54,5 @@ > add_tier_dir('js', ['js/src/ctypes/libffi'], static=True) > + if CONFIG['OS_TARGET'] == 'Android' and CONFIG['ENABLE_INTL_API'] and not CONFIG['MOZ_ANDROID_LIBSTDCXX'] and not CONFIG['MOZ_NATIVE_ICU']: > + # When building ICU as shared library, it requires static version of > + # libgabi++ before building ICU. > + add_tier_dir('js', ['build/gabi++'], static=True) static=True means external build system, which is not the case. Also, part 2 puts gabi++ traversal in build/moz.build, that should be enough, as it's before ICU. Except for JS_STANDALONE, so you'd have to deal with that.

Comment on attachment 8375389 [details] [diff] [review] Part 3. Support ICU building for Android/Gonk Review of attachment 8375389 [details] [diff] [review]: ----------------------------------------------------------------- The patch in its form is ok besides the following nits, but I need to look at this deeper to see whether it's actually fine to statically link gabi++ in each icu dynamic lib, thus keeping r? around. ::: build/autoconf/icu.m4 @@ +111,5 @@ > + if test "$OS_TARGET" = "Android" -a -z "$MOZ_ANDROID_LIBSTDCXX"; then > + # require linking libgabi++_static for RTTI support > + MOZ_ICU_LIBS='$(call EXPAND_LIBNAME_PATH,gabi++_static,$(DEPTH)/build/gabi++) $(call EXPAND_LIBNAME_PATH,$(ICU_LIB_NAMES),$(DEPTH)/intl/icu/target/lib)' > + else > + MOZ_ICU_LIBS='$(call EXPAND_LIBNAME_PATH,$(ICU_LIB_NAMES),$(DEPTH)/intl/icu/target/lib)' MOZ_ICU_LIBS='$(call EXPAND_LIBNAME_PATH,$(ICU_LIB_NAMES),$(DEPTH)/intl/icu/target/lib)' if ...; then MOZ_ICU_LIBS="$MOZ_ICU_LIBS "'$(call EXPAND_LIBNAME_PATH,gabi++_static,$(DEPTH)/build/gabi++)' fi @@ +303,5 @@ > > + if test "$OS_TARGET" = "Android" -a -z "$MOZ_ANDROID_LIBSTDCXX"; then > + ICU_CXXFLAGS="-I$_topsrcdir/build/gabi++/include $ICU_CXXFLAGS" > + # Now, gabi++ isn't built yet, so we cannot add option of # -lgabi++_static. > + ICU_LDFLAGS="$ICU_LDFLAGS -L$_objdir/build/gabi++" Remove this line. ::: intl/icu/Makefile.in @@ +67,5 @@ > ifdef CROSS_COMPILE > +$(ICU_MAKE) -j1 -C host STATIC_O=$(OBJ_SUFFIX) GENRBOPTS='-k -R -C' > endif > +ifeq ($(OS_TARGET),Android) > + +$(ICU_MAKE) -j1 -C target STATIC_O=$(OBJ_SUFFIX) GENRBOPTS='-k -R' LD_SOOPTIONS="-Wl,-Bsymbolic -lgabi++_static" And use $(call EXPAND_LIBNAME_PATH,gabi++_static,$(abspath $(DEPTH))/build/gabi++) here

Comment on attachment 8375389 [details] [diff] [review] Part 3. Support ICU building for Android/Gonk Review of attachment 8375389 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Mike Hommey [:glandium] from comment #64) > Comment on attachment 8375389 [details] [diff] [review] > Part 3. Support ICU building for Android/Gonk > > Review of attachment 8375389 [details] [diff] [review]: > ----------------------------------------------------------------- > > The patch in its form is ok besides the following nits, but I need to look > at this deeper to see whether it's actually fine to statically link gabi++ > in each icu dynamic lib, thus keeping r? around. I won't be able to tell anything without a complete patch queue, which, from the look of the build errors i get with the current one, is not the case. Clearing review flag until then. ::: build/autoconf/icu.m4 @@ +111,5 @@ > + if test "$OS_TARGET" = "Android" -a -z "$MOZ_ANDROID_LIBSTDCXX"; then > + # require linking libgabi++_static for RTTI support > + MOZ_ICU_LIBS='$(call EXPAND_LIBNAME_PATH,gabi++_static,$(DEPTH)/build/gabi++) $(call EXPAND_LIBNAME_PATH,$(ICU_LIB_NAMES),$(DEPTH)/intl/icu/target/lib)' > + else > + MOZ_ICU_LIBS='$(call EXPAND_LIBNAME_PATH,$(ICU_LIB_NAMES),$(DEPTH)/intl/icu/target/lib)' MOZ_ICU_LIBS='$(call EXPAND_LIBNAME_PATH,$(ICU_LIB_NAMES),$(DEPTH)/intl/icu/target/lib)' if ...; then MOZ_ICU_LIBS="$MOZ_ICU_LIBS "'$(call EXPAND_LIBNAME_PATH,gabi++_static,$(DEPTH)/build/gabi++)' fi @@ +303,5 @@ > > + if test "$OS_TARGET" = "Android" -a -z "$MOZ_ANDROID_LIBSTDCXX"; then > + ICU_CXXFLAGS="-I$_topsrcdir/build/gabi++/include $ICU_CXXFLAGS" > + # Now, gabi++ isn't built yet, so we cannot add option of # -lgabi++_static. > + ICU_LDFLAGS="$ICU_LDFLAGS -L$_objdir/build/gabi++" Remove this line. ::: intl/icu/Makefile.in @@ +67,5 @@ > ifdef CROSS_COMPILE > +$(ICU_MAKE) -j1 -C host STATIC_O=$(OBJ_SUFFIX) GENRBOPTS='-k -R -C' > endif > +ifeq ($(OS_TARGET),Android) > + +$(ICU_MAKE) -j1 -C target STATIC_O=$(OBJ_SUFFIX) GENRBOPTS='-k -R' LD_SOOPTIONS="-Wl,-Bsymbolic -lgabi++_static" And use $(call EXPAND_LIBNAME_PATH,gabi++_static,$(abspath $(DEPTH))/build/gabi++) here

This is still failure with --with-shared-js on Android and Gonk. But, if I add the following in intl/icu/Makefile.in, it works fine too with --with-shared-js. buildicu:: ... +$(MAKE) -C $(DEPTH)/build/gabi++ libs glandium, do you have any idea for this? If we should not support --with-shared-js on Android and Gonk, please ignore this. Or should I file a separate bug?

Comment on attachment 8380435 [details] [diff] [review] Part 3. ICU build support for Android/Gonk Review of attachment 8380435 [details] [diff] [review]: ----------------------------------------------------------------- I still can't review this without a complete patch set that works. I'm getting build errors with the current patch set.

(In reply to Mike Hommey [:glandium] from comment #67) > Comment on attachment 8380435 [details] [diff] [review] > Part 3. ICU build support for Android/Gonk > > Review of attachment 8380435 [details] [diff] [review]: > ----------------------------------------------------------------- > > I still can't review this without a complete patch set that works. I'm > getting build errors with the current patch set. What is error?

--with-intl-api doesn't work on android even if it sets at force. If you need that's patch for --with-intl-api, I will attach it

libgabi++_static.a is built on "binaries" rule of base tier. But libicu is static=True in /moz.build and Makefile has "default:: buildicu" rule, so when building libicu, base tier seems to process "compile" rule (for generating .o file) only. Is this correct? When running make default into int/icu, does all "base" tier finish lib binaries to create static library? Or how to check whether binaries rule is finished?

Has there been any progress here lately? Thanks!

Wow, all the issues this depends on appear fixed! That's good news for the 10 (!) issues dependent on this one. Any prognosis?

Any prognosis on this? Issue 922963, which is very important for RTL support, depends on this.

The community wants to use the new Intl API, but Firefox mobile is slowing adoption: http://stackoverflow.com/a/16233919/1000608 I hope this can be given more attention.

IIRC, we've given up on supporting some devices with very small available storage space after product management expressed concern for the growth of the binary size two years ago. Considering that making ICU available to Gecko consistently would allow us to *remove* a bunch of legacy duplicate functionality from Gecko, once everything is fixed, the net size change would be less than just the addition of ICU--there would be the removal of the old cruft, too. Considering the changed circumstances regarding devices we are trying to support and considering that unconditionally adding ICU allows us to remove stuff, too, can the product management assessment of the permissibility of ICU, please, be revisited?

Henri, I'm pretty sure that decision is going to need a number for what the net increase in binary size will be, so NI to you to provide that.

This is APK size of Android-11 Before without ICU / ECMA 402 38984 KB After with ICU / ECMA-402 (ftp://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/m_kato@ga2.so-net.ne.jp-d10169d9e76b/try-android-api-11/) 42777 KB

Update to 5.0.0

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #76) > Henri, I'm pretty sure that decision is going to need a number for what the > net increase in binary size will be, so NI to you to provide that. Getting that number isn't really a realistic ahead-of-time task, because it would involve developing fixes for the bugs (recursively) blocked by this one ahead of time and measuring the result. Regardless of the exact number, we should put value on being able to get rid of Netscape-era code that duplicates parts of ICU functionality here and there. The main reason for those various bits of Netscape-era code to stick around is that, due to this bug, we're not allowed to depend on ICU on all platforms. My point was that if we are no longer targeting devices where the weight added by ICU would be too much, we should just take ICU on Android and unblock all the bugs that are now blocked by the inability to rely on ICU everywhere.

We've been getting raised eyebrows from countries where APK size matters due to users having to 'pay' for the data to download the file. Increasing the APK size results in a more costly 'app' to download, increasing barrier to adoption. I agree there's value in removing old code, but here's what isn't clear to me from reading the latest comments in the bug: * I think we're saying that by removing old code and relying solely on ICU may result in a lower APK size, but Makoto's assessment looks like we're still adding to the size - thoughts here? I know it will be hard to quantity, but we can't be increasing the size without losing out on adoption (via 'costly download' of the application itself) * Are there any other gains from relying on ICU? Did I read one comment correctly whereby adoption of RTL support is easier? * Do we lose anything from relying only on ICU (other than potentially an increase in APK size)? Any other functionality impacted? Thanks, Karen

(In reply to Karen Rudnitski [:kar] from comment #83) > * I think we're saying that by removing old code and relying solely on ICU > may result in a lower APK size, but Makoto's assessment looks like we're > still adding to the size - thoughts here? Presumably Makoto's numbers come from simply turning on ICU, but not reaping the benefits of any code removals doing so may enable. So they are total worst-case numbers. Gerv

(In reply to Karen Rudnitski [:kar] from comment #83) > We've been getting raised eyebrows from countries where APK size matters due > to users having to 'pay' for the data to download the file. Increasing the > APK size results in a more costly 'app' to download, increasing barrier to > adoption. > > * Are there any other gains from relying on ICU? Did I read one comment > correctly whereby adoption of RTL support is easier? > * Do we lose anything from relying only on ICU (other than potentially an > increase in APK size)? Any other functionality impacted? You offer an opportunity for a segue, which I'll take. :-) Enabling ICU also enables the Intl API in JavaScript, exposed to webpages. This lets people create efficiently reusable date/time and number formatters and text collator instances with very particular behaviors. Old-school JS's support for any of this is practically non-existent, to the point where sites use *roundtrips to servers* to get what they want. (I need not point out that this *particularly* hurts on mobile.) More on that in https://hacks.mozilla.org/2014/12/introducing-the-javascript-internationalization-api/ that I wrote last year. So that's a gain. And along with that gain. The best argument for taking this is going to be rather blunt. In very short order every other browser engine is going to ship this. All desktop does, now. If mobile doesn't now, it will extraordinarily soon. Where the browser ships with the OS, they don't care about size. Where the browser's a separate download (Chrome, I think), they've eaten the cost and ship anyway. We will be odd man out extraordinarily soon (if we aren't already). Sites will begin using this, and sites will depend on it, almost certainly no matter what Firefox for <mobile platform> does. The download size hit is IMO surpassed by the reality that people are going to expect this, and it simply won't be acceptable much longer to not ship this. And yes, ideally we'd not add 3MB to the download size. But I don't think we have that choice any more.

> * Are there any other gains from relying on ICU? Just to add a note here: making it possible to rely on ICU everywhere would unblock bug 728180, which in turn is the reason for our failure to render Balinese text correctly with an OpenType font (see bug 1161900); this would also affect several other (relatively obscure, admittedly) writing systems. In this case, lack of an up-to-date Unicode service is hurting our ability to display content in some languages. We may add hacks to work around some of this, but wallpapering over weaknesses in our Unicode support in a piecemeal fashion really sucks when there's a well-maintained library available that would provide the services.

I hear some very compelling arguments here and I also feel that our l10n support is one of our strengths, which I wouldn't like to erode. If we are to implement this, I would absolutely want to scrub the code clean of any unnecessary code (as discussed earlier in the bug) in order to look to mitigate the increase in APK. We do have other 'fat fennec' bugs on file, but I really want to call out that this is becoming a bigger (haha) issue and any time code is introduced that adds size, we need to be really careful about the cost / benefit. I am truly hoping we can at least net out the same and not just 'add'. Therefore can we look at making the removal of the unnecessary code as dependencies to implementing this bug as a way of mitigation, and I'll also look at discussing other 'fat fennec' bugs as a higher order of priority now knowing this change will happen?

An additional note, using ICU would enable Lao to be correctly rendered, in addition to the other writing systems.

Android comes with ICU, although each version of android comes with a different version of ICU. I wonder if we could however leverage this to avoid embedding ICU.

(In reply to Mike Hommey [:glandium] from comment #89) > Android comes with ICU, although each version of android comes with a > different version of ICU. I wonder if we could however leverage this to > avoid embedding ICU. At least, our implementation requires 50.1. Althouhg JB uses 50.1, ICS still uses 4x.1. If dat file under /usr/icu is compatible with our ICU version, it may be able to share dat file and reduce APK size.

I'm going to be updating ICU this quarter. The newer ICU adds fresh APIs that'll let us fix bugs in our current implementation. For reason of features, and for reasons of safety (in staying more up-to-date wrt security), I would very much rather not try to tie us to 50.1, or to anything other than what we have in-tree. That also goes for data files. When I wrote comment 85's article, I originally had an example with Iraqi dinars. I removed it because our behavior used "IQD", not a high-quality localized symbol. And other browsers *did* use the symbol -- likely because they use newer CLDR data files/locale data. To sum up: trying to use older system ICU will degrade API support (and require extra code to negotiate the difference using symbol lookups or something), trying to use older system locale data will degrade observable locale-data quality. Do we *really* have that much justification for Android as lone straggler?

> Do we *really* have that much justification for > Android as lone straggler? As Karen mentioned, our APK size is already a problem on a number of fronts -- enough that we split our builds, will probably do so again to win a few hundred KB, and are actively exploring excluding fonts, hyphenation dictionaries, and other such assets from the download. It's worth noting that there's an absolute hard limit on APK size of 50MB. Fx38 is about 34MB, 39 and 40 will be bigger. libxul.so alone is now bigger than Fennec 14's entire APK! At current baseline growth rates we have maybe another year or two before we are forced to use out-of-band downloads. Our efforts at slimming Fennec down are barely treading water. In the last week alone I've heard requests to add 5 or 6MB of features, quite apart from the usual stream of paper cuts as an entire world of APIs is shoveled into omni.ja and libxul. Without a very high bar for these big bumps, we would be in trouble, so I hope you understand our pushback: if everyone could simply land their important feature unmodified ("but *this feature* is important enough that 3MB is acceptable!"), Fennec would simply be too large to ship at all. That's not to say that this feature doesn't meet that bar -- but there's a natural expectation on our part that significant effort has been expended to minimize the size impact prior to landing, and even more effort prior to reaching Beta. That can include downloading assets OTA, killing obsolete code, whatever… but APK size is enormously important, so work needs to be done. (In reply to Henri Sivonen (:hsivonen) from comment #82) > My point was that if we are no longer targeting devices where the > weight added by ICU would be too much, we should just take ICU on Android > and unblock all the bugs that are now blocked by the inability to rely on > ICU everywhere. Unfortunately, 30-40MB OTA (and way more than that on-disk) is still somewhere in that no-man's land of "too much" -- fine on 64GB devices with unlimited plans, utterly unsuitable for a large portion of our global addressable user base. It's certainly not the case that we've excluded all devices that wouldn't be able to handle this space increase: all space increases are bad, 3MB is particularly bad, and I wouldn't feel particularly comfortable until we were 5MB *smaller* than we are now.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #91) > trying to use older system locale data will degrade observable > locale-data quality. Presumably only to the quality which the rest of the system is exhibiting. It it possible to work out what proportion of any size increase would be code, and what proportion would be locale data? Gerv

(In reply to Richard Newman [:rnewman] from comment #92) > Our efforts at slimming Fennec down are barely treading water. What kind of efforts? I don't see a bug (open or fixed) or a dev-platform discussion about compressing the PNG assets, omni.ja or the APK itself using Zopfli. Are we already using Zopfli? (If not, shouldn't we do that as the very first step?) Also, is there now an effort to remove legacy XPCOM interfaces that aren't not used by the Android front end and have limited utility in extensions (the assumption being that the Android extension legacy is more constrained than the desktop extension legacy anyway)? > Without a very high bar for these big bumps Part of the problem with focusing on big bumps is that it creates a disincentive against improving a lot of things in one go. ICU has applicability in a number of places around the code base but is impractical to land ICU itself piecemeal such that each of the point of applicability would fly under the "big bump" radar. In that sense, ICU gets penalized despite its broad utility compared to a bunch of small improvements each one of which would not look like a "big bump". In some sense, ICU is too good to be permitted in the product. It's also worth noting that efforts to slim the APK won't really help the case for ICU. Logically, any slimming that's possible before landing ICU is not the kind of slimming that's blocked on ICU. Therefore, even if some stuff is removed first, it won't count in favor of ICU as if there was some ICU-specific quota that any slimming counted towards. Instead, ICU will still have the size that it had before and the debate in this bug will be the same as before some slimming took place. But back to the issue of desktop and mobile being different: What brought me here was that I was trying to remove some 90s legacy code and two people, emk and I, both first diagnosed that code removal being blocked by the use of a Windows 95-era API in the JS engine. Only later I noticed that the Windows 95-era code in JS engine has become dead code thanks to desktop, including Windows, now using ICU. The old code is just sticking around confusing developers because the "ICU unavailable" code paths have not been removed yet due to this bug remaining unfixed. Now, it would be possible to identify a set of changes that would allow us to simplify legacy code using the knowledge that whenever "ICU unavailable" is true, it is also true that we are running on an Android/Gonk OS. But it doesn't seem like a very good use of time to do that kind of work when the real fix would be making ICU available on all platforms and then removing the resulting dead code unconditionally. Having to treat mobile as different in terms of what Gecko internals can rely on and having non-mobile decoy dead code stick around pending this bug getting fixed both seem bad for developer productivity and for addressing technical debt in our code base.

Before we can take on this feature that we know will increase our APK size (which we simply cannot absorb without cost of users), we need to know that we'll either net out with the change (by removing other code) or we have made progress on other initiatives to slim down the APK (these are outlined here: https://wiki.mozilla.org/Mobile/Fatfennec). If there are other initiatives that we can add to the list, please definitely file them. But until we know that we're not going to increase our APK size for this feature, we simply can't take the risk of having this ever-increasing barrier. It's been called out by numerous partners as being a problem, and it's one we unfortunately cannot ignore.

Several "unused" XUL files has been removed by some my bugs. Also, we can remove unicode data table such as bug 1170072 if landing ICU support on all platform. Also play store's packages is release build only. So packages size becomes smaller than nightly/aurora. x86 m-c package is 45MB now (arm is 42MB), but even if landing this, it will increment ~2.5MB with bug 1170072 landing.

(In reply to Henri Sivonen (:hsivonen) from comment #94) > (In reply to Richard Newman [:rnewman] from comment #92) > > Our efforts at slimming Fennec down are barely treading water. > > What kind of efforts? Bug 942609. > I don't see a bug (open or fixed) or a dev-platform > discussion about compressing the PNG assets, omni.ja or the APK itself using > Zopfli. Please file one (or several)! > Also, is there now an effort to remove legacy XPCOM > interfaces that aren't not used by the Android front end and have limited > utility in extensions (the assumption being that the Android extension > legacy is more constrained than the desktop extension legacy anyway)? I'd be very happy to see the platform team take that on. (The frontend team does when we can, but it's much slower going when we drive.) There's some work (both completed and outstanding) in Bug 1044079, and some more stuff in Bug 1044289. > It's also worth noting that efforts to slim the APK won't really help the > case for ICU… Instead, ICU will > still have the size that it had before and the debate in this bug will be > the same as before some slimming took place. That's only one way to look at it. Another is to say: you just got back from the grocery store with two bags of groceries. Your refrigerator is of fixed size and is almost full. You need to take some stuff out of the refrigerator before you can put the groceries in. > Having to treat mobile as different in terms of what Gecko internals can > rely on and having non-mobile decoy dead code stick around pending this bug > getting fixed both seem bad for developer productivity and for addressing > technical debt in our code base. Oh, I quite agree! You don't need to convince me of the value. My only concern is that we have a small refrigerator.

(In reply to Richard Newman [:rnewman] from comment #97) > Please file one (or several)! Filed Bug 1173894.

rebase and update reftest

Comment on attachment 8601353 [details] [diff] [review] Part 1. Import libgabi++ from https://android.googlesource.com/platform/abi/cpp/ Review of attachment 8601353 [details] [diff] [review]: ----------------------------------------------------------------- Pick up android code of android-5.0.0_r1 to use rtti support since stlport doesn't have it. gerv, this library is from AOSP. (I mistake previous comment). Should I update license.html file?

Comment on attachment 8601355 [details] [diff] [review] Part 2. Add moz.build etc to build gabi++ for Android Add moz.build to build gabi++

Comment on attachment 8601356 [details] [diff] [review] Part 3. Add build config of ICU for Android build ICU for android. If using MOZ_ANDROID_LIBSTDCXX, it is unnecessary to add gabi++ path.

Comment on attachment 8601353 [details] [diff] [review] Part 1. Import libgabi++ from https://android.googlesource.com/platform/abi/cpp/ No need to update license.html for files from AOSP; it already has the necessary entries. Gerv

Nathan tells me that libc++ is ready to be used on Android, we might as well use that (and he said he would land it next week) So I guess part 1 & 2 are not needed anymore, and part 3 needs an update.

Static library version of both libc++ and libstdc++ has rtti support. So after moztlport is removed, we don't need additional libraries to build ICU on Android.

rebased part 3

rebased

With these patches and the toolchain from bug 1201310 I get an android build with a working ICU and Intl. API.

We need that for b2gdroid, for which we use a toolchain that can unwind.

Comment on attachment 8601355 [details] [diff] [review] Part 2. Add moz.build etc to build gabi++ for Android Review of attachment 8601355 [details] [diff] [review]: ----------------------------------------------------------------- javascript:void(0);

Try build with this patch set: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12d00ec74a53

Comment on attachment 8601353 [details] [diff] [review] Part 1. Import libgabi++ from https://android.googlesource.com/platform/abi/cpp/ Review of attachment 8601353 [details] [diff] [review]: ----------------------------------------------------------------- The older patch corresponding to this one that had a r+ had a README.mozilla file saying where this comes from.

Comment on attachment 8601355 [details] [diff] [review] Part 2. Add moz.build etc to build gabi++ for Android Review of attachment 8601355 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/gabi++/README.mozilla @@ +1,1 @@ > +This copy of libgabi++ is from Android 5.0 source code on https://android.googlesource.com/platform/abi/cpp/. Tag is android-5.0.0_r1. Oh, that's where the README went... please move it back to the first patch. ::: build/gabi++/moz.build @@ +5,5 @@ > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +Library('gabi++') > + > +FORCE_STATIC_LIB = True This line is effectively a no-op. You should be able to remove it. @@ +32,5 @@ > +DISABLE_STL_WRAPPING = True > +NO_VISIBILITY_FLAGS = True > + > +CXXFLAGS += [ > + '-fexceptions', with this here, is the Makefile.in change necessary?

Comment on attachment 8665025 [details] [diff] [review] Part 3. Add build config of ICU for Android Review of attachment 8665025 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/moz.build @@ +16,5 @@ > DIRS += ['unix'] > > if CONFIG['OS_TARGET'] == 'Android' and not CONFIG['MOZ_ANDROID_LIBSTDCXX']: > DIRS += ['stlport'] > + if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android' and CONFIG['ENABLE_INTL_API']: CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android' is pretty much guaranteed by CONFIG['OS_TARGET'] == 'Android' above. ::: config/external/icu/moz.build @@ +20,5 @@ > if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk': > OS_LIBS += [ > 'gabi++', > ] > + if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android' and not CONFIG['MOZ_ANDROID_LIBSTDCXX']: This will need a rebase on top of bug 1164921, which I want to land independently of whether or not we actually switch to libc++.

Comment on attachment 8673785 [details] [diff] [review] Part 4. Turn on Intl API for b2gdroid only Review of attachment 8673785 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ -9117,5 @@ > > -# Internationalization isn't built or exposed by default in non-desktop > -# builds. Bugs to enable: > -# > -# Android: bug 864843 This comment still applies, except the status of the bug is now confusing... File a new bug for android and reference it here?

(In reply to Mike Hommey [:glandium] from comment #114) > Comment on attachment 8665025 [details] [diff] [review] > Part 3. Add build config of ICU for Android > > Review of attachment 8665025 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: build/moz.build > @@ +16,5 @@ > > DIRS += ['unix'] > > > > if CONFIG['OS_TARGET'] == 'Android' and not CONFIG['MOZ_ANDROID_LIBSTDCXX']: > > DIRS += ['stlport'] > > + if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android' and CONFIG['ENABLE_INTL_API']: > > CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android' is pretty much guaranteed by > CONFIG['OS_TARGET'] == 'Android' above. Isn't CONFIG['OS_TARGET'] == 'Android' also true for MOZ_WIDGET_TOOLKIT == gonk ? > ::: config/external/icu/moz.build > @@ +20,5 @@ > > if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk': > > OS_LIBS += [ > > 'gabi++', > > ] > > + if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android' and not CONFIG['MOZ_ANDROID_LIBSTDCXX']: > > This will need a rebase on top of bug 1164921, which I want to land > independently of whether or not we actually switch to libc++. I don't understand what you mean here. We can land this patch before bug 1164921 is ready, right?

(In reply to [:fabrice] Fabrice Desré from comment #116) > (In reply to Mike Hommey [:glandium] from comment #114) > > Comment on attachment 8665025 [details] [diff] [review] > > Part 3. Add build config of ICU for Android > > > > Review of attachment 8665025 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: build/moz.build > > @@ +16,5 @@ > > > DIRS += ['unix'] > > > > > > if CONFIG['OS_TARGET'] == 'Android' and not CONFIG['MOZ_ANDROID_LIBSTDCXX']: > > > DIRS += ['stlport'] > > > + if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android' and CONFIG['ENABLE_INTL_API']: > > > > CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android' is pretty much guaranteed by > > CONFIG['OS_TARGET'] == 'Android' above. > > Isn't CONFIG['OS_TARGET'] == 'Android' also true for MOZ_WIDGET_TOOLKIT == > gonk ? Indeed. My bad. > > ::: config/external/icu/moz.build > > @@ +20,5 @@ > > > if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk': > > > OS_LIBS += [ > > > 'gabi++', > > > ] > > > + if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android' and not CONFIG['MOZ_ANDROID_LIBSTDCXX']: > > > > This will need a rebase on top of bug 1164921, which I want to land > > independently of whether or not we actually switch to libc++. > > I don't understand what you mean here. We can land this patch before bug > 1164921 is ready, right? That's the thing. Bug 1164921 *is* ready to land, independently of actually switching the default. And it has an impact on MOZ_ANDROID_LIBSTDCXX.

(In reply to Mike Hommey [:glandium] from comment #113) > Comment on attachment 8601355 [details] [diff] [review] > Part 2. Add moz.build etc to build gabi++ for Android > > Review of attachment 8601355 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: build/gabi++/README.mozilla > @@ +1,1 @@ > > +This copy of libgabi++ is from Android 5.0 source code on https://android.googlesource.com/platform/abi/cpp/. Tag is android-5.0.0_r1. > > Oh, that's where the README went... please move it back to the first patch. > > ::: build/gabi++/moz.build > @@ +5,5 @@ > > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > > + > > +Library('gabi++') > > + > > +FORCE_STATIC_LIB = True > > This line is effectively a no-op. You should be able to remove it. > > @@ +32,5 @@ > > +DISABLE_STL_WRAPPING = True > > +NO_VISIBILITY_FLAGS = True > > + > > +CXXFLAGS += [ > > + '-fexceptions', > > with this here, is the Makefile.in change necessary? Do you prefere something like http://mxr.mozilla.org/mozilla-central/source/build/unix/elfhack/Makefile.in#6 ?

> > @@ +32,5 @@ > > > +DISABLE_STL_WRAPPING = True > > > +NO_VISIBILITY_FLAGS = True > > > + > > > +CXXFLAGS += [ > > > + '-fexceptions', > > > > with this here, is the Makefile.in change necessary? > > Do you prefere something like > http://mxr.mozilla.org/mozilla-central/source/build/unix/elfhack/Makefile. > in#6 ? To clarify, I think the moz.build part is enough and the Makefile.in part unnecessary. But I could be wrong.

Updated to add the readme.

Addressing comments.

Addressing comments.

Addressing comments.

Comment on attachment 8674572 [details] [diff] [review] Part 1. Import libgabi++ from https://android.googlesource.com/platform/abi/cpp/ Review of attachment 8674572 [details] [diff] [review]: ----------------------------------------------------------------- I guess you forgot to git/hg add, because the README is not in here.

Comment on attachment 8674573 [details] [diff] [review] Part 2. Add moz.build etc to build gabi++ for Androidntl-2.patch Review of attachment 8674573 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/gabi++/Makefile.in @@ +1,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this file, > +# You can obtain one at http://mozilla.org/MPL/2.0/. > + > +include $(topsrcdir)/config/rules.mk A Makefile.in with only an include of config/rules.mk is useless :) Please remove it.

Comment on attachment 8674574 [details] [diff] [review] Part 3. Add build config of ICU for Android Review of attachment 8674574 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/autoconf/icu.m4 @@ +272,5 @@ > fi > > if test -n "$gonkdir"; then > ICU_CXXFLAGS="-I$gonkdir/abi/cpp/include $ICU_CXXFLAGS" > + elif test "$OS_TARGET" = Android -a -z "$MOZ_ANDROID_LIBSTDCXX"; then MOZ_ANDROID_LIBSTDCXX was removed in bug 1164921, so this is the wrong test. ::: config/external/icu/moz.build @@ +20,5 @@ > if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk': > OS_LIBS += [ > 'gabi++', > ] > + if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android' and not CONFIG['MOZ_ANDROID_LIBSTDCXX']: likewise.

In the future, could people please open a fresh bug for something like this, or clone the existing one, rather than appropriate an existing bug for a different purpose? All the old comments related to Android specifically (not b2gdroid), and we're kind of losing them with this hijacking. (And on a lesser note, I had this number nicely memorized. :-) )

Adding the missing README.

Removed useless Makefile.in

Updated use of MOZ_ANDROID_LIBSTDCXX to MOZ_ANDROID_CXX_STL

Pushed to try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=7302a1a70614

https://hg.mozilla.org/integration/b2g-inbound/rev/a6e94fdb528e https://hg.mozilla.org/integration/b2g-inbound/rev/ba7fd2ff5fa0 https://hg.mozilla.org/integration/b2g-inbound/rev/baa0579f8a9e https://hg.mozilla.org/integration/b2g-inbound/rev/6e34bc916dda

https://hg.mozilla.org/mozilla-central/rev/a6e94fdb528e https://hg.mozilla.org/mozilla-central/rev/ba7fd2ff5fa0 https://hg.mozilla.org/mozilla-central/rev/baa0579f8a9e https://hg.mozilla.org/mozilla-central/rev/6e34bc916dda