Closed
Bug 1346341
Opened 8 years ago
Closed 7 years ago
OSPreferences doesn't link ifndef ENABLE_INTL_API
Categories
(Core :: Internationalization, enhancement)
Core
Internationalization
Tracking
()
RESOLVED
WORKSFORME
Tracking | Status | |
---|---|---|
firefox55 | --- | affected |
People
(Reporter: bzbarsky, Assigned: zbraniecki)
Details
Attachments
(2 files)
2.24 KB,
patch
|
Details | Diff | Splinter Review | |
59 bytes,
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
It's calling various functions that are defined in the per-platform .cpp files that are only built ifdef ENABLE_INTL_API.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years ago
|
||
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)
Reporter | ||
Comment 3•8 years ago
|
||
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...
Assignee | ||
Comment 4•8 years ago
|
||
: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 5•8 years ago
|
||
mozreview-review |
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+
Comment 6•8 years ago
|
||
(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)
Comment 7•8 years ago
|
||
(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)
Assignee | ||
Comment 8•8 years ago
|
||
:sfink, :ted, ping?
Assignee | ||
Comment 9•7 years ago
|
||
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.
Description
•