Sync DevTools plural-form.js with intl/PluralForm.jsm
Categories
(DevTools :: General, enhancement, P2)
Tracking
(firefox67 fixed, firefox68 fixed)
People
(Reporter: flod, Assigned: flod)
References
Details
Attachments
(1 file)
|
47 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
DevTools has a copy of PluralFors.jsm in
The problem is that nobody is maintaining, and it gets out of sync (looks like I was the last one to touch in bug 1441146).
|
Assignee | |
Comment 1•6 years ago
|
||
|
||
Comment 2•6 years ago
|
||
Thanks Francesco. Julian is on PTO until the end of this week I believe, so he'll be able to review this once he gets back. Sorry about the delay.
|
|
Assignee | |
Comment 3•6 years ago
|
||
We should get this in 67, and I'd like to avoid requesting for uplift to beta. Can someone else take the review this week?
|
||
Comment 5•6 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 6•6 years ago
|
||
Comment on attachment 9049469 [details]
Bug 1533688 - Sync DevTools plural-form.js with intl/PluralForm.jsm (add rule #19)
Beta/Release Uplift Approval Request
- Feature/Bug causing the regression: None
- User impact if declined: Plural strings in some languages (Russian is the most notable one) will use the incorrect form. While this code is not covered by tests, the same change done to the general /intl module is.
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: Yes
- 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): Only affecting 3 locales, and only for the DevTools portion.
- String changes made/needed: None
|
||
Comment 7•6 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #6)
Comment on attachment 9049469 [details]
Bug 1533688 - Sync DevTools plural-form.js with intl/PluralForm.jsm (add rule #19)Beta/Release Uplift Approval Request
- Feature/Bug causing the regression: None
- User impact if declined: Plural strings in some languages (Russian is the most notable one) will use the incorrect form. While this code is not covered by tests, the same change done to the general /intl module is.
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: Yes
- 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): Only affecting 3 locales, and only for the DevTools portion.
- String changes made/needed: None
Flod, the patch adds support for "// 19: Bosnian, Croatian, Serbian" while the uplift request mentions Russian as a notable target, just making sure that the uplift does include what's actually needed.
|
|
Assignee | |
Comment 8•6 years ago
|
||
(In reply to Pascal Chevrel:pascalc from comment #7)
Flod, the patch adds support for "// 19: Bosnian, Croatian, Serbian" while the uplift request mentions Russian as a notable target, just making sure that the uplift does include what's actually needed.
Sorry, for some reason my brain shuffled things around (we removed those 3 locales from the Russian rule).
While the impact of these 3 locales is much smaller, we should still be uplifting, because the code fall backs to rule 0 (Chinese) if the plural rule is out of bounds
https://hg.mozilla.org/mozilla-central/file/f0bfe6ecffc8/devtools/shared/plural-form.js#l129
So, English text with plurals would fall back to the single case.
|
|
||
Comment 9•6 years ago
|
||
Comment on attachment 9049469 [details]
Bug 1533688 - Sync DevTools plural-form.js with intl/PluralForm.jsm (add rule #19)
Fix for plurals in our l10n library, small and well understood patch already verified in Nightly, approved for beta 5, thanks.
|
||
Comment 10•6 years ago
|
||
| bugherder uplift | ||
Description
•