1343766 - Reimplement non-ICU DateTimeFomatter

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement)

Description

[Makoto Kato [:m_kato]]

By bug 1329841, this is removed unfortunately

But we still need it until INTL_API options is removed completely. (From product team request. see bug 1343725)

Comment 1

[Makoto Kato [:m_kato]]

It requires minimal implementation for English only since Android's bionic supports English only.

Comment 2

[Masatoshi Kimura [:emk]]

We can restore DateTimeFormatUnix.cpp that bug 1329841 removed. We don't have to implement kDateFormatYearMonth and kDateFormatWeekday because it will be used only for Android and therefore Thunderbird is not relevant.

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Add DateTimeFomat using C locale without ICU

Comment 4

[Makoto Kato [:m_kato]]

Comment on attachment 8845228 [details] [diff] [review]
Add DateTimeFomat using C locale without ICU

Fennec requires non-ICU version of DateTimeFormat.  Since Bionic only supports C locale, fallback version uses C locale only since we want that nsIPlatformCharset is deprecated.

Comment 5

[Masatoshi Kimura [:emk]]

Comment on attachment 8845228 [details] [diff] [review]
Add DateTimeFomat using C locale without ICU

Review of attachment 8845228 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/locale/DateTimeFormatFallback.cpp
@@ +23,5 @@
> +    char* locale = setlocale(LC_TIME, nullptr);
> +    if (locale) {
> +      mLocale.Assign(locale);
> +    }
> +    setlocale(LC_TIME, aLocale);

Do we have to call setlocale at all even though Bionic supports only C locale?

Comment 6

[Makoto Kato [:m_kato]]

(In reply to Masatoshi Kimura [:emk] from comment #5)
> Comment on attachment 8845228 [details] [diff] [review]
> Add DateTimeFomat using C locale without ICU
> 
> Review of attachment 8845228 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: intl/locale/DateTimeFormatFallback.cpp
> @@ +23,5 @@
> > +    char* locale = setlocale(LC_TIME, nullptr);
> > +    if (locale) {
> > +      mLocale.Assign(locale);
> > +    }
> > +    setlocale(LC_TIME, aLocale);
> 
> Do we have to call setlocale at all even though Bionic supports only C
> locale?

If this is android only, we don't need setlocale.  I can remove this restore RAII class

Comment 7

[Masatoshi Kimura [:emk]]

Comment on attachment 8845228 [details] [diff] [review]
Add DateTimeFomat using C locale without ICU

Review of attachment 8845228 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/locale/DateTimeFormatFallback.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Please rename this file to DateTimeFormatAndroid.cpp or something to make it explicit that this is designed only for Android.

@@ +14,5 @@
> +namespace mozilla {
> +
> +nsCString* DateTimeFormat::mLocale = nullptr;
> +
> +class MOZ_RAII AutoTimeLocaleRestore final

(In reply to Makoto Kato [:m_kato] from comment #6)
> If this is android only, we don't need setlocale.  I can remove this restore
> RAII class

Let's remove it.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc293981e2a3
Add DateTimeFomat without ICU for Android. r=emk

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fc293981e2a3

Comment 10

[Makoto Kato [:m_kato]]

Attachment: for aurora

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1329841 and 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]:
No

[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 8846462 [details] [diff] [review]
for aurora

Need to support turning off Intl API and ICU. Aurora54+.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8acbaea575e2