Closed Bug 1325459 Opened 3 years ago Closed 3 years ago

Expose PluralRules on mozIntl

Categories

(Core :: Internationalization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: zbraniecki, Assigned: Waldo)

Details

Attachments

(2 files)

With bug 1270146 fixed, we should now expose PluralRules on mozIntl for internal use until we get it exposed on Intl for web content.
Ugh, sorry to bother you :waldo, but I have no idea how to approach this. So far we've only exposed functions on mozIntl, never objects, and the way `AddIntlExtras` handled them was via `JS_SELF_HOSTED_FN`, which won't work here I believe.

Instead, we need to get sth like:

RootedObject pluralRulesProto(cx, CreatePluralRulesPrototype(cx, intl, global));
global->setReservedSlot(PLURAL_RULES_PROTO, ObjectValue(*pluralRulesProto));

but in AddIntlExtras we don't have global, and we also don't have `CreatePluralRulesPrototype` or PLURAL_RULES_PROTO in js.cpp.

How should I approach it?
Flags: needinfo?(jwalden+bmo)
Comment on attachment 8821462 [details]
Bug 1325459 - Expose PluralRules via addIntlExtras.

I hacked and slashed my way through the code to get it in the jsshell.cpp.

I still don't know if you're ok with leaving it exposed (we don't have any plans to change the API and we do intend to push it to stage4 in January), or would you prefer to move it to behind addIntlExtras and mozIntl.

If the latter, I'll also need a bit of help in getting the CreatePluralRulesPrototype and PLURAL_RULES_PROTO in http://searchfox.org/mozilla-central/source/toolkit/components/mozintl/MozIntl.cpp - and I don't know how to get it.
Attachment #8821462 - Flags: feedback?(jwalden+bmo)
The existing patch mostly gets the job done, but I'm not happy about exposing global object slots, nor about the awkward interface of requiring callers to manually fill in a slot after PluralRules creation.  And there's the mozIntl thing as well.  This should cover all the bases.
Attachment #8821649 - Flags: review?(andrebargull)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Comment on attachment 8821462 [details]
Bug 1325459 - Expose PluralRules via addIntlExtras.

https://reviewboard.mozilla.org/r/100758/#review101408

It's a good start, but I want slightly different interface boundaries -- we should expose PluralRules-adding functionality by a function, not by a function to create it all and the implicit step of setting the reserved slot.  I took the patch and massaged things into the right form -- one that also handles mozIntl pretty easily, too.
Attachment #8821462 - Flags: review-
Comment on attachment 8821462 [details]
Bug 1325459 - Expose PluralRules via addIntlExtras.

sweet, thanks a lot Waldo!
Attachment #8821462 - Flags: feedback?(jwalden+bmo)
Comment on attachment 8821649 [details] [diff] [review]
Somewhat-different tack

Review of attachment 8821649 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me as long as it's not necessary to install PluralRules to two objects (mozIntl and Intl). 

To get Intl.PluralRules to stage 4, we need two shipping implementations [1], but with this change we only make PluralRules available internally. Is this intended?

[1] https://tc39.github.io/process-document/
Attachment #8821649 - Flags: review?(andrebargull) → review+
The separate installation modes are for the shell, and separately for browser chrome code that wants to do plural stuff.  Shouldn't be any overlap of uses.  And the mozIntl idea is "you give me an object, I create and put the PluralRules constructor on it", so it'd only go on that one object.

This is maybe fuzzy, but I think we interpret "shipping" to mean "the functionality can be invoked in some way and tested for correctness to the extent possible".  Back in the day, a prefixed implementation would have counted, even though it wasn't exactly the same signature in how you accessed it.  Intl.js the polyfill counts as an implementation, even tho (obviously) using it requires an extra step that ESmumble doesn't have.  Making someone call |addIntlExtras(Intl)| before doing stuff is in the same vein, IMO.
Flags: needinfo?(jwalden+bmo)
Yeah, I'm pretty sure that MozIntl works as a mean to stage 4. The goal of having two implementation is to test if the algo can be implemented and catch any gotchas in the process. We've done it.
I'll recommend exposing it on Intl once we have stage 4.
Ah okay, I see. My understanding of "Significant in-the-field experience with shipping implementations [...]" was that a proposed feature is implemented and also exposed to web-content (or available when a about:config flag is set).
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4dd4f25303c
Expose the PluralRules constructor only by explicit action,, for now.  r=anba
https://hg.mozilla.org/mozilla-central/rev/b4dd4f25303c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.