Closed Bug 1343766 Opened 3 years ago Closed 3 years ago

Reimplement non-ICU DateTimeFomatter

Categories

(Core :: Internationalization, enhancement)

Unspecified
Android
enhancement
Not set

Tracking

()

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

People

(Reporter: m_kato, Assigned: m_kato)

References

Details

Attachments

(2 files)

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)
It requires minimal implementation for English only since Android's bionic supports English only.
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 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.
Attachment #8845228 - Flags: review?(VYV03354)
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?
(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 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.
Attachment #8845228 - Flags: review?(VYV03354) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc293981e2a3
Add DateTimeFomat without ICU for Android. r=emk
https://hg.mozilla.org/mozilla-central/rev/fc293981e2a3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Attached patch for auroraSplinter Review
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
Attachment #8846462 - Flags: approval-mozilla-aurora?
Comment on attachment 8846462 [details] [diff] [review]
for aurora

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