Closed
Bug 1504656
Opened 6 years ago
Closed 6 years ago
Locale fallback handling for Intl.RelativeTimeFormat doesn't work correctly
Categories
(Core :: JavaScript: Internationalization API, defect)
Core
JavaScript: Internationalization API
Tracking
()
RESOLVED
FIXED
mozilla66
People
(Reporter: anba, Assigned: anba)
References
Details
Attachments
(1 file)
4.61 KB,
patch
|
Waldo
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
ICU bug: https://unicode-org.atlassian.net/browse/ICU-20253
Test case:
---
addIntlExtras(Intl);
print(new Intl.RelativeTimeFormat("ak").format(1, "second"));
---
Expected:
- Prints "+1 s"
Actual:
- Throws "Error: internal error while computing Intl data"
Assignee | ||
Comment 1•6 years ago
|
||
ICU4C doesn't fallback to StandardPlural::OTHER (cf. ICU4J at [1]) when no formatter was found, which leads to reporting U_INVALID_FORMAT_ERROR in [2].
[1] https://github.com/unicode-org/icu/blob/e1e5f363a09144aac359b1b68552bb25e4251fd9/icu4j/main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java#L784-L793
[2] https://searchfox.org/mozilla-central/rev/3564dfde3b125878dc5a04fe92629fc5195942df/intl/icu/source/i18n/reldatefmt.cpp#896,898
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #9029203 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 2•6 years ago
|
||
Setting the tracking flag for Firefox 65, because this issue blocks bug 1504334, which in turn is slated for Firefox 65 and already checked in.
tracking-firefox65:
--- → ?
Comment 4•6 years ago
|
||
Waldo, can you please prioritize this review since it needs to be uplifted to 65 and we're already nearly past the early beta stage?
Flags: needinfo?(jwalden)
Comment 5•6 years ago
|
||
Comment on attachment 9029203 [details] [diff] [review]
bug1504656.patch
Review of attachment 9029203 [details] [diff] [review]:
-----------------------------------------------------------------
::: intl/icu/source/i18n/reldatefmt.cpp
@@ +165,5 @@
> + while (true) {
> + int32_t style = fStyle;
> + do {
> + if (relativeUnitsFormatters[style][unit][pastFutureIndex][pluralUnit] != nullptr) {
> + return relativeUnitsFormatters[style][unit][pastFutureIndex][pluralUnit];
eeeeugh that is horrible repetition but must not get sniped changing imported code
Attachment #9029203 -
Flags: review?(jwalden) → review+
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53d23de684a2
Add patch to workaround ICU bug in RelativeDateTimeCacheData::getRelativeDateTimeUnitFormatter. r=jwalden
Comment 7•6 years ago
|
||
Comment on attachment 9029203 [details] [diff] [review]
bug1504656.patch
[Beta/Release Uplift Approval Request]
Feature/Bug causing the regression: Bug 1504334
User impact if declined: Certain sorts of relative-time formatting operations will throw an inscrutable internal error. I don't know how many locales are susceptible to this problem, to say precisely how extensive the impact is.
Is this code covered by automated tests?: Yes
Has the fix been verified in Nightly?: No
Needs manual test from QE?: No
If yes, steps to reproduce:
List of other uplifts needed: None
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): Unlike most of our local ICU patches, this one hasn't been taken upstream yet. That isn't entirely ideal.
However, the rationale for the change as given in the ICU ticket seems straightforward, and it looks like this is just redoing an operation with inputs that can already be triggered other ways, so it doesn't seem to open up new risk.
String changes made/needed: N/A
Flags: needinfo?(jwalden)
Attachment #9029203 -
Flags: approval-mozilla-beta?
Comment 8•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Comment 9•6 years ago
|
||
Comment on attachment 9029203 [details] [diff] [review]
bug1504656.patch
[Triage Comment]
Fixes a bug in ICU which causes some relative-time formatting operations to throw an error. Approved for 65.0b8.
Attachment #9029203 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 10•6 years ago
|
||
bugherder uplift |
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•