1344596 - Don't build OSPreferences service without ENABLE_INTL_API

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1344596 - Don't build OSPreferences API without ENABLE_INTL_API.

We need keep --with-intl-api=no setting.  So we shouldn't build OSPreferences API without ENABLE_INTL_API that uses ICU.

Review commit: https://reviewboard.mozilla.org/r/117374/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117374/

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

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?)?

Comment 3

[Makoto Kato [: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.

Comment 4

[Makoto Kato [:m_kato]]

Attachment: Bug 1344596 - Allow OSPreferences API without ENABLE_INTL_API.

Since we have to keep --with-intl-api=no setting, we allow OSPrefrences API wiout ENABLE_INTL_API.

Review commit: https://reviewboard.mozilla.org/r/117454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117454/

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

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?

Comment 6

[Makoto Kato [:m_kato]]

(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.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ok! I'm fine with any way you want the config flags to work. I'm also fine with getting OSPreferences_android separated out.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9055693f792d
Allow OSPreferences API without ENABLE_INTL_API. r=gandalf

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9055693f792d

Comment 10

[Makoto Kato [:m_kato]]

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

Comment 11

[Gerry Chang [:gchang]]

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+.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/556ffc2114f7