Closed Bug 1344596 Opened 3 years ago Closed 3 years ago

Don't build OSPreferences service without ENABLE_INTL_API

Categories

(Core :: Internationalization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → m_kato
Can we be a bit smarter here and not disable OSPreferences? I'd like to use it for GetSystemLocales which don't require INTL_API.

Can we just disable the API to return null/false for API calls that require Intl API (GetDateTimeFormat at the moment?)?
Flags: needinfo?(m_kato)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)
> Can we be a bit smarter here and not disable OSPreferences? I'd like to use
> it for GetSystemLocales which don't require INTL_API.
> 
> Can we just disable the API to return null/false for API calls that require
> Intl API (GetDateTimeFormat at the moment?)?

I have to reimplement non-ICU DateTimeFormat for C locale only in Gecko 54 since Android's bionic only supports C locale.  Although OSPreferences::ReadSystemLocales returns BCP47 string, we don't have a generator from current LANG environment value to BCP47.  But I will use nsPosixLocale::GetXPLocale for it.

But, OK.  We will keep OS preferences API even if ENABLE_INTL_API is turned off.
Flags: needinfo?(m_kato)
Attachment #8843795 - Flags: review?(gandalf)
Attachment #8843795 - Attachment is obsolete: true
Comment on attachment 8843846 [details]
Bug 1344596 - Allow OSPreferences API without ENABLE_INTL_API.

https://reviewboard.mozilla.org/r/117454/#review119086

Looks good to me! Thank you.

I left two suggestions which are up to you.

::: intl/locale/tests/unit/xpcshell.ini:27
(Diff revision 1)
>  [test_pluralForm_english.js]
>  [test_pluralForm_makeGetter.js]
>  
>  [test_localeService.js]
>  [test_osPreferences.js]
> +skip-if = toolkit == "android" # bug 1344596

Is there a way to skip-if on ENABLE_INTL_API? This would allow me to turn on intl API on my branch and have those tests run on android.

If that's anyhow not readily available, then let's not put work into this.

::: intl/locale/unix/OSPreferences_unix.cpp:15
(Diff revision 1)
>  using namespace mozilla::intl;
>  
>  bool
>  OSPreferences::ReadSystemLocales(nsTArray<nsCString>& aLocaleList)
>  {
> +#ifdef ENABLE_INTL_API

would you want to do the same ifdefs on in bindings for mac/windows/gtk so that we can turn intl on/off on any platform? Or are we ok just tailoring the exception for android?
Attachment #8843846 - Flags: review?(gandalf) → review+
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5)
> Comment on attachment 8843846 [details]
> Bug 1344596 - Allow OSPreferences API without ENABLE_INTL_API.
> 
> https://reviewboard.mozilla.org/r/117454/#review119086
> 
> Looks good to me! Thank you.
> 
> I left two suggestions which are up to you.
> 
> ::: intl/locale/tests/unit/xpcshell.ini:27
> (Diff revision 1)
> >  [test_pluralForm_english.js]
> >  [test_pluralForm_makeGetter.js]
> >  
> >  [test_localeService.js]
> >  [test_osPreferences.js]
> > +skip-if = toolkit == "android" # bug 1344596
> 
> Is there a way to skip-if on ENABLE_INTL_API? This would allow me to turn on
> intl API on my branch and have those tests run on android.

There is no option for ENABLE_INTL_API now.  If we want it, we have to add it to mozinfo such as mozinfo.intl.
 
> If that's anyhow not readily available, then let's not put work into this.
> 
> ::: intl/locale/unix/OSPreferences_unix.cpp:15
> (Diff revision 1)
> >  using namespace mozilla::intl;
> >  
> >  bool
> >  OSPreferences::ReadSystemLocales(nsTArray<nsCString>& aLocaleList)
> >  {
> > +#ifdef ENABLE_INTL_API
> 
> would you want to do the same ifdefs on in bindings for mac/windows/gtk so
> that we can turn intl on/off on any platform? Or are we ok just tailoring
> the exception for android?

No, I would like to remove --with-intl-api option on desktop version (windows, macOS and gtk) at finally.  And we should rename OSPreferences_unix.cpp to OSPreferences_android.cpp since our supported widgets are windows, macOS (Cocoa), gtk and android only.  I will file a follow up bugs.
Ok! I'm fine with any way you want the config flags to work. I'm also fine with getting OSPreferences_android separated out.
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/9055693f792d
Allow OSPreferences API without ENABLE_INTL_API. r=gandalf
https://hg.mozilla.org/mozilla-central/rev/9055693f792d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8843846 [details]
Bug 1344596 - Allow OSPreferences API without ENABLE_INTL_API.

Approval Request Comment
[Feature/Bug causing the regression]:
bug 1343725

[User impact if declined]:
We cannot release Firefox for android on beta and release channel since we cannot build Fennec on beta/release builder.

:snorp and :blassey decides that we still keep turning off Intl API and ICU on Fennec beta/release channel.  So we require this fix to build Fennec on release and beta channel.

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
No, this is build issue.

[List of other uplifts needed for the feature/fix]:
Nothing.

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
This is build issue only.

[String changes made/needed]:
No
Attachment #8843846 - Flags: approval-mozilla-aurora?
Comment on attachment 8843846 [details]
Bug 1344596 - Allow OSPreferences API without ENABLE_INTL_API.

Need to support turning off Intl API and ICU on Fennec beta/release channel. Aurora54+.
Attachment #8843846 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1343725
You need to log in before you can comment on or make changes to this bug.