Closed Bug 1349470 Opened 3 years ago Closed 3 years ago

Date.toLocaleFormat is corrupted after landing bug 1346674

Categories

(Core :: JavaScript: Internationalization API, defect)

55 Branch
Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: m_kato, Assigned: zbraniecki)

References

Details

(Keywords: regression)

Attachments

(1 file)

- Env
Windows 10 with Japanese locale setting

- Step
Run the following on Scratchpad.

var d = new Date();
d.toLocaleFormat();

- Result
/*
2017”N3ŒŽ22“ú 16:53:14
*/

- Expected Result
/*
2017年3月22日 16:53:14
*/
Blocks: 1346674
No longer blocks: 1349454
Summary: Date.toLocaleFormat is corrupted after landing bug 1349454 → Date.toLocaleFormat is corrupted after landing bug 1346674
:pike - Similar question to bug 1349454, but about dates instead of fonts.

With ICU and Intl API we use browser locale to localize the date/time. Should `toLocaleFormat` follow OS locale or browser locale?
Flags: needinfo?(l10n)
I think we're getting a japanese date format, but get the encoding wrong here.

No idea where that happens.
Flags: needinfo?(l10n)
The encoding is wrong because it asks for:
http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/js/xpconnect/src/XPCLocale.cpp#162

which if (fortunately) only used by:
http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/js/src/jsdate.cpp#2705

so it'll all go away when we get rid of toLocaleFormat, but in the meantime we have a situation where the whole JS context uses app's locale, except of this one function which takes the OS locale.

Ok, I'll revert it, because we only use toLocaleFormat in Android now, which is a non-ICU scenario, so we have no other place but OS to take the datetime tokens from. :(
Comment on attachment 8850068 [details]
Bug 1349470 - Use OS locale for unicode conversions for Date.toLocaleFormat.

https://reviewboard.mozilla.org/r/122788/#review125186

::: js/xpconnect/src/XPCLocale.cpp:177
(Diff revision 1)
> -      NS_ConvertUTF8toUTF16 localeStr(appLocale);
> +      // access to any other date/time than the OS one, so we have to also
> +      // use the OS locale for unicode conversions.
> +      // See bug 1349470 for more details.
> +      nsAutoCString osLocale;
> +      OSPreferences::GetInstance()->GetSystemLocale(osLocale);
> +      NS_ConvertUTF8toUTF16 localeStr(osLocale);

Since we don't use NS_ILOCALE_DATE, we can use NS_CopyNativeToUnicode simply instead of LocaleService, nsIPlatformCharset and EncodingUtils.

    nsAutoString result;
    NS_CopyNativeToUnicode(nsDependentCString(src), result);
    JSString* ucstr =
      JS_NewUCStringCopyN(cx, result.get(), result.Length());
    if (ucstr) {
      rval.setString(ucstr);
      return true;
    }
    
But it should be handled by another bug, so it is OK for this bug by your fix.
Attachment #8850068 - Flags: review?(m_kato) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1038d1c502be
Use OS locale for unicode conversions for Date.toLocaleFormat. r=m_kato
Blocks: 943283
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/1038d1c502be
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.