Status

()

enhancement
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

({dev-doc-complete})

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments)

Intl.PluralRules reached Stage 4 at today's TC39 meeting.

We can now expose it on the public Intl object!
(Assignee)

Updated

2 years ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8912806 - Flags: review?(andrebargull)
Attachment #8912807 - Flags: review?(andrebargull)
(Assignee)

Comment 3

2 years ago
Andre, can you take a look at this?

I'd also appreciate any tips on why would this be failing at the moment on test262 tests despite the fact that I did expose it and the tests should now be passing unconditionally.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #3)
> I'd also appreciate any tips on why would this be failing at the moment on
> test262 tests despite the fact that I did expose it and the tests should now
> be passing unconditionally.

The test262 importer adds code to manually install PluralRules into the global Intl object, and I guess with the current patch it probably just replaces Intl.PluralRules with the `undefined` value. I think the following changes should make test262 work again:

- Delete js/src/tests/test262-intl-extras.js 
- Remove these two lines http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/js/src/tests/test262-update.py#324-325
- Remove all lines in js/src/tests/test262/intl402/PluralRules/shell.js (but don't delete the file itself!)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
 thank you :)
try looks green!

Comment 10

2 years ago
mozreview-review
Comment on attachment 8912806 [details]
Bug 1403318 - Expose Intl.PluralRules.

https://reviewboard.mozilla.org/r/184114/#review190086

LGTM!

The `addIntlExtras` guard and call should also be removed from this file: http://searchfox.org/mozilla-central/rev/20e41d4a61a8f5e34c9cf357304b78b3e9bced8a/js/src/tests/Intl/PluralRules/supportedLocalesOf.js#1,365

And the `"Calling this function more than once in a realm/global will throw."` warning for addIntlExtras's help message can now be removed, too. http://searchfox.org/mozilla-central/rev/20e41d4a61a8f5e34c9cf357304b78b3e9bced8a/js/src/shell/js.cpp#6695-6696


Not directly related to this change, but it looks like the `PluralRulesSelect` entry in vm/CommonPropertyNames.h is no longer needed: http://searchfox.org/mozilla-central/rev/20e41d4a61a8f5e34c9cf357304b78b3e9bced8a/js/src/vm/CommonPropertyNames.h#287
Attachment #8912806 - Flags: review?(andrebargull) → review+

Comment 11

2 years ago
mozreview-review
Comment on attachment 8912807 [details]
Bug 1403318 - Remove mozIntl.PluralRules.

https://reviewboard.mozilla.org/r/184116/#review190090

The changes look reasonable to me, but since I don't really know how idl-files are processed, this is more like a rs=me than a proper r=me :-)
Attachment #8912807 - Flags: review?(andrebargull) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

2 years ago
Comment on attachment 8912807 [details]
Bug 1403318 - Remove mozIntl.PluralRules.

Jonathan, can you support anba's review with yours? It's a pretty simple change to remove the special treatment of mozIntl.PluralRules since we now can expose it as Intl.PluralRules.
Attachment #8912807 - Flags: review?(jfkthame)
(Assignee)

Comment 15

2 years ago
Anba: I went ahead and incorporated all three of your feedback suggestions into the patch. Hope it's ok with you :)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #15)
> Anba: I went ahead and incorporated all three of your feedback suggestions
> into the patch. Hope it's ok with you :)

Cool, thanks! :-)
Comment on attachment 8912807 [details]
Bug 1403318 - Remove mozIntl.PluralRules.

Seems fine to me; sorry for the delay.
Attachment #8912807 - Flags: review?(jfkthame) → review+

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0b2271bd6e50
https://hg.mozilla.org/mozilla-central/rev/72af2f1919fe
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.