Closed Bug 1346341 Opened 7 years ago Closed 7 years ago

OSPreferences doesn't link ifndef ENABLE_INTL_API

Categories

(Core :: Internationalization, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox55 --- affected

People

(Reporter: bzbarsky, Assigned: zbraniecki)

Details

Attachments

(2 files)

It's calling various functions that are defined in the per-platform .cpp files that are only built ifdef ENABLE_INTL_API.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Makoto, I wrote this patch to unblock :bz, but I also vaguely remember that you wanted to remove --disable-icu from non-android platforms.

If that's the case, then maybe adding more ifdefs doesn't make sense?
Flags: needinfo?(m_kato)
I didn't us --disable-icu.  I used --without-intl-api.

I'd be even happier if we made --enable-shared-js link even with intl api...
:sfink, :glandium - :waldo and hg blame suggest that you may be able to help us here.

Can we get intl api to link with --enable-shared-js?
Flags: needinfo?(sphink)
Flags: needinfo?(mh+mozilla)
Comment on attachment 8846114 [details]
Bug 1346341 - Ifdef more OSPreferences platform-specific calls that depend on ICU.

https://reviewboard.mozilla.org/r/119200/#review121218

This is sad, but I guess we can do it for now.

::: intl/locale/mac/OSPreferences_mac.cpp:100
(Diff revision 1)
>  bool
>  OSPreferences::ReadDateTimePattern(DateTimeFormatStyle aDateStyle,
>                                     DateTimeFormatStyle aTimeStyle,
>                                     const nsACString& aLocale, nsAString& aRetVal)
>  {
> +#ifdef ENABLE_INTL_API

This doesn't look necessary, AFAICT -- nothing in this method depends on ICU, it's all just Core Foundation calls.
Attachment #8846114 - Flags: review?(jfkthame) → review+
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)
> Makoto, I wrote this patch to unblock :bz, but I also vaguely remember that
> you wanted to remove --disable-icu from non-android platforms.
> 
> If that's the case, then maybe adding more ifdefs doesn't make sense?

Firefox's frontend already uses Intl.* APIs into /browser.  They doesn't have fallback like Fennec's toolkit code when ICU is disabled.  Also, some people (ex. TPE's datetime picker for desktop) into layout uses ICU from JS/XUL/XBL.

So, When using ICU option for desktop, some features except to JS's Intl API won't work well.  Although it is no matter that I keep the option, developer should make sense that some features is broken.
Flags: needinfo?(m_kato)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #4)
> :sfink, :glandium - :waldo and hg blame suggest that you may be able to help
> us here.
> 
> Can we get intl api to link with --enable-shared-js?

This fails with:
error: /home/glandium/gecko-push/obj-x86_64-pc-linux-gnu/js/src/../../config/external/icu/common/udata.o: requires dynamic R_X86_64_PC32 reloc against 'icudt58_dat' which may overflow at runtime; recompile with -fPIC

because libmozjs.so doesn't link the file that contain the icudt58_dat symbol... but js/src/moz.build kind of does that on purpose for reasons mentioned there. I don't remember the details, but maybe Ted does. If it's important that the data is not linked to the shell, fixing this will require splitting the build of libmozjs and libjs_static.
Flags: needinfo?(mh+mozilla) → needinfo?(ted)
:sfink, :ted, ping?
closing, since we removed all ENABLE_INTL_API from intl/* code
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(ted)
Flags: needinfo?(sphink)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: