We need a PluralRules API module based on CLDR data [0]. Depending on the outcome of bug 1270140 we may want to write it for spidermonkey, or as a module. [0] https://dxr.mozilla.org/mozilla-central/source/intl/icu/source/data/misc/plurals.txt

Comment on attachment 8762604 [details] [diff] [review] WIP v1 Review of attachment 8762604 [details] [diff] [review]: ----------------------------------------------------------------- Waldo, I'm working on implementing the Intl.PluralRules spec. I'll eventually want to expose it for chrome code first and use in Firefox, but initially wanted to write it as anew formatter. The current version of the spec is here: https://rawgit.com/zbraniecki/intl-plural-rules-spec/intl-numformatter/index.html I kept the code as simple as possible for now and just tried to get it working, which it does! Now I'd like to plug it into ICU and I don't know how. Need your help ::: js/src/builtin/Intl.cpp @@ +2502,5 @@ > + MOZ_ASSERT(args.length() == 0); > + > + RootedValue result(cx); > + //XXX: We should be getting PluralRules locales, not NumberFormat ones > + if (!intl_availableLocales(cx, unum_countAvailable, unum_getAvailable, &result)) How can I get the availableLocales for PluralRules from ICU? ::: js/src/builtin/Intl.js @@ +3011,5 @@ > + //XXX: Temporary hack > + let locale = requestedLocales[0]; > + pluralRules['[[Locale]]'] = locale; > + > + pluralRules['[[PluralRule]]'] = pluralRulesFn[t][locale]; How can I get the right PluralRule from ICU? I guess I'll have to call to C++ code, so I probably won't be able to resolve the function in the initializer, but rather (similarly to the spec), will just carry Type and Locale and in ResolvePlural will call to `PluralRuleSelection (locale, type, n, operands)` which will call C++ which will call the right ICU function, but need help identifying how to write the C++ part.

I assume we're looking for : 1) http://icu-project.org/apiref/icu4j/com/ibm/icu/text/PluralRules.html#getAvailableULocales() 2) http://icu-project.org/apiref/icu4j/com/ibm/icu/text/PluralRules.html#select(double) but I can't find it in C++ ICU code

:waldo, ping?

Comment on attachment 8762604 [details] [diff] [review] WIP v1 Review of attachment 8762604 [details] [diff] [review]: ----------------------------------------------------------------- I still need to look at Intl.js, and I'm certain I'll have comments there (the "[[]]" conceit is definitely unnecessary, note how we've never adhered to it in Intl.js elsewhere). But this will hopefully at least give you something to start with, and an idea how this functionality should IMO be exposed. ::: js/src/builtin/Intl.cpp @@ +2383,5 @@ > +PluralRules(JSContext* cx, const CallArgs& args, bool construct) > +{ > + RootedObject obj(cx); > + > + if (!construct) { Rather than doing things ECMA-402 1st ed.-style as far as non-constructing calls go, we'll want to do things a more ES6-style way. Something like RootedObject proto(cx); if (!GetPrototypeFromCallableConstructor(cx, args, &proto)) return false; RootedObject obj(cx, NewObjectWithGivenProto(cx, &PluralRulesClass, proto)); ... should do, once the |constructing| observation later on is taken into account. @@ +2414,5 @@ > + obj->as<NativeObject>().setReservedSlot(UPLURAL_RULES_SLOT, PrivateValue(nullptr)); > + } > + > + RootedValue locales(cx, args.length() > 0 ? args[0] : UndefinedValue()); > + RootedValue options(cx, args.length() > 1 ? args[1] : UndefinedValue()); Those can just be args.get(0) and args.get(1) now. @@ +2437,5 @@ > + CallArgs args = CallArgsFromVp(argc, vp); > + MOZ_ASSERT(args.length() == 2); > + // intl_PluralRules is an intrinsic for self-hosted JavaScript, so it > + // cannot be used with "new", but it still has to be treated as a > + // constructor. This function shouldn't be necessary if we're adding/exposing this via the JS_* API I noted. And if you remove that, you can just use args.isConstructing() to determine constructing-ness in the creation function. @@ +2502,5 @@ > + MOZ_ASSERT(args.length() == 0); > + > + RootedValue result(cx); > + //XXX: We should be getting PluralRules locales, not NumberFormat ones > + if (!intl_availableLocales(cx, unum_countAvailable, unum_getAvailable, &result)) It doesn't appear there's any way to get available locales in the public API. uplrules_countAvailable and such would be in upluralrules.h if it existed, but they're not there. Comparing against what the other Intl object types do to see if we can cheat and depend on internal things: unum_countAvailable and such typically delegate to an internal variety like Collator::getAvailableLocales or so. We can find that plurrules.h *does* have PluralRules::getAvailableLocales, tho -- which calls into PluralAvailableLocalesEnumeration stuff in intl/icu/source/i18n/plurrule.cpp. So basically, implementing locale-counting/enumerating APIs is going to require patching ICU, calling the existing APIs where necessary, and likely adding new code to do so a bit as well. (Counting the number of locales, for example, shouldn't require enumerating all of them.) One tricky bit: PluralRules::getAvailableLocales is #ifndef U_HIDE_INTERNAL_API. I *think* that means it'll be available, but it's most definitely unstable, non-dependable API, and there's a very good chance it'll break --with-system-icu or whatever it is. Proceed cautiously. @@ +2568,5 @@ > return nullptr; > if (!InitDateTimeFormatClass(cx, Intl, global)) > return nullptr; > + if (!InitPluralRulesClass(cx, Intl, global)) > + return nullptr; To add this but not make it part of the default exposed functionality yet, we're going to not want this. Instead we'll want to have extern JS_PUBLIC_API(bool) JS_GetIntlPluralRulesConstructor(JSContext* cx); in jsfriendapi.h, and then in Intl.cpp we'll want bool JS_GetIntlPluralRulesConstructor(JSContext* cx) { } that's basically just InitPluralRulesClass, but without the Intl argument (we won't define on the Intl object) and without the global argument (just use cx->global()) and returns the constructor. Then, to expose this at all, we can have a module with one function that simply calls and returns JS_DefinePluralRulesConstructor. The process for having such a module, should be something vaguely like what toolkit/components/ctypes/ctypes.cpp does to expose ctypes functionality. I'm handwaving here -- this bit isn't my strong suit, but I could fake it up from reading the existing code, for sure.

:srl - can we add uplrules_countAvailable to ICU?

Comment on attachment 8762604 [details] [diff] [review] WIP v1 Review of attachment 8762604 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/Intl.js @@ +1161,5 @@ > function setLazyData(internals, type, lazyData) > { > assert(internals.type === "partial", "can't set lazy data for anything but a newborn"); > + assert(type === "Collator" || type === "DateTimeFormat" || > + type == "NumberFormat" || type === "PluralRules", "bad type"); Mild preference for assertion message on its own line when the assertion condition spans multiple lines, for readability. Also in the second hunk here. @@ +2891,5 @@ > > +/********** Intl.PluralRules **********/ > + > +function GetOperands(s) { > + let n = ToNumber(s); General formatting here (two-space indentation) is all off, just to note. Also, algorithms are going to want step comment-numbering -- and along with that, some whitespace between conceptually separate steps. This largely all sort of blends together into one long mess, mostly. :-\ @@ +2901,5 @@ > + if (dp === -1) { > + iv = n; > + } else { > + iv = callFunction(String_substring, s, 0, dp - 1); > + fv = callFunction(String_substring, s, dp); Because |dp| is already range-checked against the string length, I think SubstringKernel the intrinsic is probably preferred for these. @@ +2905,5 @@ > + fv = callFunction(String_substring, s, dp); > + f = ToNumber(fv); > + v = ToLength(fv.length); > + } > + i = callFunction(std_Math_abs, ToNumber(iv)); std_Math_abs is Math.abs, so it computes the absolute value of an *argument*. callFunction is for calling functions that act upon |this|. It's not necessary for functions that don't examine |this|. So this should just be std_Math_abs(ToNumber(iv)). (And maybe not even the ToNumber bit, because I think std_Math_abs does that internally, but we'd have to check that.) @@ +2921,5 @@ > + result['[[IntegerDigits]]'] = i; > + result['[[NumberOfFractionDigits]]'] = v; > + result['[[NumberOfFractionDigitsWithoutTrailing]]'] = w; > + result['[[FractionDigits]]'] = f; > + result['[[FractionDIgitsWithoutTrailing]]'] = t; The idea of the Intl algorithms isn't that they be exactly implemented in all characteristics, including storing internal properties under their direct names even if they're never used. The internal properties, in particular, are characteristics of the *algorithm* for pluralizing, or formatting, or collating. It's not necessarily the case that they need to be, or *should* be, literally stored. This looks like a place where that's the case. @@ +2946,5 @@ > + > + > +function InitializePluralRules(pluralRules, locales, options) { > + > + let pluralRulesFn = { This object (which incidentally you're not allowed to created using an object literal, because then it has Object.prototype on its prototype chain, and so various accesses into it have a chance of becoming user-visible) is where you should be storing *all* of the internal guts of plural-rules behavior that you need. You should be initializeIntlObject with it basically only at the *end* of the function, not so early. @@ +2996,5 @@ > + resolvedOptions() { > + return no; > + } > + } > + pluralRules['[[NumberFormat]]'] = nf; I could be mistaken, but I don't think you need to do all this work to lazily store NumberFormat details like this. Since you're just (it appears) doing things "as if by |new NumberFormat|", you can just *do* that (or |new std_Intl_NumberFormat| or whatever), then rely on *that* constructor's implementation to compute everything lazily. Definitely we shouldn't be duplicating laziness here. @@ +3030,5 @@ > +function Intl_PluralRules_resolvedOptions() { > + let pluralRules = this; > + > + let no = callFunction( > + pluralRules['[[NumberFormat]]'].resolvedOptions, The point of resolved options is that they're what was *actually* used -- not even just what we determined was requested after parsing arguments and such. Just regurgitating that parsed info doesn't really do the job. We'll need to use ICU APIs that expose the actual used information -- or add them if they don't exist, or use internal unstable stuff, either of which given upluralrules.h/plurrules.h's state might be necessary. This function accepts an untrusted |this|, then starts trying to access '[[]]' properties on it. Those properties (if they're being accessed correctly) must therefore have been defined visible to script. That's not cool. Any sort of internal property like this, should have been stashed in the internals object corresponding to |this|. Then that internals object should be retrieved via a getPluralRulesInternals function, that *necessarily* validates |this| before assuming it's a NumberFormat, or a PluralRules, or a Collator, &c. This is one big reason for having internals-object gunk, past the perf aspects.

Mass change dependency tree for bug 1279002 into a whiteboard keyword.

A major update to the patch: - I unbitrot it and got it to match the current codebase - I applied most of the feedback from :waldo - I applied most of the changes to the latest version of the spec There are several things I still struggle with: 1) I need to get ResolveLocale to work 2) I need to figure out how to apply the spec change https://github.com/tc39/ecma402/commit/2f9226d2d913c12e62cf0e9909401fcba638cae2 3) I need to add pluralCategories 4) I need to add availableLocales for PluralRules from ICU. In the process I discovered one more limitation of PluralRules spec and filed ecma402 to PR that I already applied in this patch: - https://github.com/tc39/ecma402/pull/107

Also, for the (4) I filed http://bugs.icu-project.org/trac/ticket/12756 and :srl suggested to temporarly use UPLURAL_TYPE_COUNT

:srl also suggested using `uloc_countAvailable` / `uloc_getAvailable` to retrieve the list of locales until we have an API specific for PluralRules and PluralRules::getKeywords() for pluralCategories.

:waldo on IRC: gandalf: you should be able to cache one; DateTimeFormat caches a UDateFormat* in a private slot, you can do similar see UDATE_FORMAT_SLOT and JSCLASS_HAS_RESERVED_SLOTS(DATE_TIME_FORMAT_SLOTS_COUNT) and how all that works

Ok, First attempt to get this patch reviewed. I'm asking for feedback and not r? because once this looks good I'll need to: - move it to mozIntl - add tests But I believe that the patch is ready to get a full review as it's fully operational and matches the spec: https://github.com/tc39/proposal-intl-plural-rules including the upcoming change - https://github.com/tc39/proposal-intl-plural-rules/issues/16

This is a close-to-final version of the patch. I unbitrotted it, aligned it with the latest version of the spec (plus https://github.com/tc39/ecma402/pull/107 which is necessary), and wrote the first chunk of test262 tests - https://github.com/tc39/test262/pull/782 ). I still need to remove it from Intl and move to mozIntl and put behind the flag and unless we're ready to start using test262,:waldo will probably want me to port some of the tests to our internal test infra. But I believe that the functionality is complete and the whole code should be reviewed now.

Comment on attachment 8806132 [details] Bug 1270146 - Add PluralRules API. https://reviewboard.mozilla.org/r/89634/#review89372 ::: js/src/builtin/Intl.h:229 (Diff revision 1) > intl_FormatDateTime(JSContext* cx, unsigned argc, Value* vp); > > +/******************** PluralRules ********************/ > + > +/** > + * Returns a new instance of the standard built-in PluralRules constructor. This comment looks very wrong. Isn't this just the implementation of Intl.PluralRules the function, *not* a function that returns Intl.PluralRules? Or is that what you meant, but you really would have been better off with s/instance of.*constructor/PluralRules instance/? ::: js/src/builtin/Intl.h:239 (Diff revision 1) > + */ > +extern MOZ_MUST_USE bool > +intl_PluralRules(JSContext* cx, unsigned argc, Value* vp); > + > +/** > + * Returns an object indicating the supported locales for number formatting "for *number formatting*"? ::: js/src/builtin/Intl.h:251 (Diff revision 1) > +extern MOZ_MUST_USE bool > +intl_PluralRules_availableLocales(JSContext* cx, unsigned argc, Value* vp); > + > +/** > + * Returns a plural rule for the number x according to the effective > + * locale and the formatting options of the given PluralRules. Most readers aren't going to have any idea what a "plural rule" is here, so you should give a brief sketch/definition of what it is after this sentence -- something like, "one of the strings 'one', 'two', 'many'..." or so. ::: js/src/builtin/Intl.h:259 (Diff revision 1) > + */ > +extern MOZ_MUST_USE bool > +intl_SelectPluralRule(JSContext* cx, unsigned argc, Value* vp); > + > +/** > + * Returns a list of plural rules categories for a given Return an array of This is something like ["one", "many"], right? Give an example of what this would return for a set of arguments. ::: js/src/builtin/Intl.cpp:36 (Diff revision 1) > #include "unicode/udat.h" > #include "unicode/udatpg.h" > #include "unicode/uenum.h" > #include "unicode/unum.h" > #include "unicode/ustring.h" > +#include "unicode/upluralrules.h" This isn't alphabetical, and it'll likely make |make -C $objdir/js/src check-style| fail, which will make treeherder scream bloody murder. ::: js/src/builtin/Intl.cpp:1481 (Diff revision 1) > + * > + */ > +static UNumberFormat* > +NewUNumberFormatForPluralRules(JSContext* cx, HandleObject pluralRules) > +{ > + RootedValue value(cx); Move this to just before the first use of |value|. ::: js/src/builtin/Intl.cpp:1510 (Diff revision 1) > + &value)) > + { > + return nullptr; > + } > + uMinimumSignificantDigits = int32_t(value.toNumber()); > + if (!GetProperty(cx, internals, internals, cx->names().maximumSignificantDigits, Blank line before this. ::: js/src/builtin/Intl.cpp:1523 (Diff revision 1) > + &value)) > + { > + return nullptr; > + } > + uMinimumIntegerDigits = int32_t(value.toNumber()); > + if (!GetProperty(cx, internals, internals, cx->names().minimumFractionDigits, Blank line before this. ::: js/src/builtin/Intl.cpp:1529 (Diff revision 1) > + &value)) > + { > + return nullptr; > + } > + uMinimumFractionDigits = int32_t(value.toNumber()); > + if (!GetProperty(cx, internals, internals, cx->names().maximumFractionDigits, Blank line before this. ::: js/src/builtin/Intl.cpp:2782 (Diff revision 1) > +static void > +pluralRules_finalize(FreeOp* fop, JSObject* obj) > +{ > + MOZ_ASSERT(fop->onMainThread()); > + > + const Value& slot = obj->as<NativeObject>().getReservedSlot(UPLURAL_RULES_SLOT); Please include the comment citing bug 949220 that's in the other finalizer functions. ::: js/src/builtin/Intl.cpp:2811 (Diff revision 1) > + return nullptr; > + > + if (!JS_DefineFunctions(cx, proto, pluralRules_methods)) > + return nullptr; > + > + Extra blank line. ::: js/src/builtin/Intl.cpp:2836 (Diff revision 1) > +{ > + CallArgs args = CallArgsFromVp(argc, vp); > + MOZ_ASSERT(args.length() == 0); > + > + RootedValue result(cx); > + // We're going to use ULocale availableLocales as per ICU recommendation Complete sentences as comments should be punctuated accordingly, i.e. a period at the end of the sentence. However, if you're going to claim there's a recommendation being followed, you should provide a link to it. And if you do that, I'd tack on ':' and then the link in angle brackets, followed by that period. ::: js/src/builtin/Intl.cpp:2856 (Diff revision 1) > + if (!nf) > + return false; > + > + ScopedICUObject<UNumberFormat, unum_close> closeNumberFormat(nf); > + > + RootedValue value(cx); Move this definition to just above its first use. ::: js/src/builtin/Intl.cpp:2873 (Diff revision 1) > + return false; > + JSAutoByteString type(cx, value.toString()); > + if (!type) > + return false; > + > + UErrorCode status = U_ZERO_ERROR; Move this down to just before its first use. ::: js/src/builtin/Intl.cpp:2886 (Diff revision 1) > + // before we push the result to PluralRules > + // > + // This should be fixed in ICU 59 and we'll be able to switch to that > + // API: http://bugs.icu-project.org/trac/ticket/12763 > + // > + bool success = intl_FormatNumber(cx, nf, x, &fmtNumValue); Um, why isn't this RootedValue fmtNumValue(cx); if (!intl_FormatNumber(...)) return false; rather than being a set-but-not-used variable? ::: js/src/builtin/Intl.cpp:2889 (Diff revision 1) > + // API: http://bugs.icu-project.org/trac/ticket/12763 > + // > + bool success = intl_FormatNumber(cx, nf, x, &fmtNumValue); > + RootedString fmtNumValueString(cx, fmtNumValue.toString()); > + AutoStableStringChars stableChars(cx); > + stableChars.initTwoByte(cx, fmtNumValueString); You're not error-checking this. ::: js/src/builtin/Intl.cpp:2892 (Diff revision 1) > + RootedString fmtNumValueString(cx, fmtNumValue.toString()); > + AutoStableStringChars stableChars(cx); > + stableChars.initTwoByte(cx, fmtNumValueString); > + > + const UChar* UfmtNumValue = Char16ToUChar(stableChars.twoByteRange().start().get()); > + uint32_t UfmtNumValueLength = u_strlen(UfmtNumValue); Don't name variables starting with a capital letter. You shouldn't need the length computation here -- |stableChars.twoByteRange().length()| should do the trick. ::: js/src/builtin/Intl.cpp:2895 (Diff revision 1) > + > + const UChar* UfmtNumValue = Char16ToUChar(stableChars.twoByteRange().start().get()); > + uint32_t UfmtNumValueLength = u_strlen(UfmtNumValue); > + > + UFormattable* fmt = unum_parseToUFormattable(nf, > + nullptr, Bad formatting of the call. ::: js/src/builtin/Intl.cpp:2901 (Diff revision 1) > + UfmtNumValue, > + UfmtNumValueLength, > + 0, > + &status); > + > + double y = ufmt_getDouble(fmt, &status); Usually you wouldn't need an error-check after this -- but it seems this function *doesn't* follow the usual ICU convention of returning garbage if the incoming status is a failure. So you need to handle failure after unum_parseToUFormattable. That said...what's the point of this call? You use |y| only once, far below here -- and if you look at that use, it *should* be one of two uses (the other being in the second uplrules_select call), if it even makes sense to use it. But you're not using it that second time. So this doesn't make any sense. ::: js/src/builtin/Intl.cpp:2908 (Diff revision 1) > + UPluralType category; > + > + if (equal(type, "cardinal")) { > + category = UPLURAL_TYPE_CARDINAL; > + } else { > + category = UPLURAL_TYPE_ORDINAL; Assert type is "ordinal" here. ::: js/src/builtin/Intl.cpp:2914 (Diff revision 1) > + } > + > + UPluralRules* pr = uplrules_openForType( > + icuLocale(locale.ptr()), > + category, > + &status); You haven't handled failure here. ::: js/src/builtin/Intl.cpp:2942 (Diff revision 1) > + if (!str) > + return false; > + > + result.setString(str); > + > + args.rval().set(result); You don't need a |result| variable at all here. Just do |args.rval().setString(str);|. ::: js/src/builtin/Intl.cpp:2967 (Diff revision 1) > + UPluralType category; > + > + if (equal(type, "cardinal")) { > + category = UPLURAL_TYPE_CARDINAL; > + } else { > + category = UPLURAL_TYPE_ORDINAL; This should assert that |type| is "ordinal". ::: js/src/builtin/Intl.cpp:2985 (Diff revision 1) > + // We should get a C API for that in ICU 59 and switch to it > + // http://bugs.icu-project.org/trac/ticket/12772 > + // > + UEnumeration* ue = uenum_openFromStringEnumeration( > + ((icu::PluralRules*)pr)->getKeywords(status), > + &status Break this up into two function calls. // if s_c doesn't work, use reinterpret_cast<> icu::StringEnumeration\* kwenum = static_cast<icu::PluralRules\*>(pr)->getKeywords(status); // This adopts |kwenum| even in case of failure. UEnumeration\* ue = uenum_openFromStringEnumeration(kwenum, status); if (U_FAILURE(status)) \{ ... \} ::: js/src/builtin/Intl.cpp:2994 (Diff revision 1) > + return false; > + } > + > + ScopedICUObject<UEnumeration, uenum_close> closeEnum(ue); > + > + ScopedICUObject<UPluralRules, uplrules_close> closePluralRules(pr); This ScopedICUObject should go immediately after the U_FAILURE check immediately following the opening of |pr|. ::: js/src/builtin/Intl.cpp:3000 (Diff revision 1) > + > + RootedObject res(cx, NewDenseEmptyArray(cx)); > + if (!res) > + return false; > + > + uint32_t count = uenum_count(ue, &status); uenum_count "can be very expensive", and we're just consuming the entire enumerator after this, so there's no reason to use it. Instead just have an infinite loop that runs until uenum_next returns null without failure. RootedString val(cx); RootedValue element(cx); while (true) { int32_t catSize; const char* cat = uenum_next(ue, &catSize, &status); if (U_FAILURE(status)) { ... } JSString* str = NewStringCopyN<CanGC>(cx, UCharToChar16(cat), AssertedCast<uint32_t>(catSize)); if (!str) return false; element.setString(str); if (!DefineElement(...)) return false; } ::: js/src/builtin/Intl.cpp:3013 (Diff revision 1) > + if (U_FAILURE(status)) { > + JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_INTERNAL_INTL_ERROR); > + return false; > + } > + > + RootedString val(cx, JS_NewStringCopyZ(cx, cat)); uenum_next returns the string length via outparam, and there's no reason to throw it away to have JS_NewStringCopyZ recompute it. Fill in a length via that outparam, then use NewStringCopyN<CanGC> instead of this function. Also move declarations of |val| and |element| out of the loop. ::: js/src/builtin/Intl.js:25 (Diff revision 1) > intl_availableCalendars: false, > intl_patternForSkeleton: false, > intl_FormatDateTime: false, > + intl_SelectPluralRule: false, > + intl_GetPluralCategories: false, > + intl_GetCalendarInfo: false I guess a comma after this so this line doesn't have to be touched in subsequent additions. ::: js/src/builtin/Intl.js:1299 (Diff revision 1) > function setLazyData(internals, type, lazyData) > { > assert(internals.type === "partial", "can't set lazy data for anything but a newborn"); > - assert(type === "Collator" || type === "DateTimeFormat" || type == "NumberFormat", "bad type"); > + assert(type === "Collator" || type === "DateTimeFormat" || > + type == "NumberFormat" || type === "PluralRules", > + "bad type"); Align the open-quote with the "t" in |type|, one column right of the opening parenthesis of the call. ::: js/src/builtin/Intl.js:1351 (Diff revision 1) > if (IsObject(internals)) { > assert(callFunction(std_Object_hasOwnProperty, internals, "type"), "missing type"); > var type = internals.type; > - assert(type === "partial" || type === "Collator" || type === "DateTimeFormat" || type === "NumberFormat", "unexpected type"); > + assert(type === "partial" || type === "Collator" || > + type === "DateTimeFormat" || type === "NumberFormat" || type === "PluralRules", > + "unexpected type"); Same alignment thing. ::: js/src/builtin/Intl.js:1887 (Diff revision 1) > internalProps = resolveNumberFormatInternals(internals.lazyData); > setInternalProperties(internals, internalProps); > return internalProps; > } > > +function CalculateNumberFormatDigitOptions(options, style, cDigits) { This'll be a separate spec helper function eventually, right? (Please rename to CalculateDigitOptions, since this is no longer NumberFormat-specific.) I'm very much un-enamored with the idea of permanently losing the step numbering comments. Nor am I happy about losing them temporarily. Please add a comment at the top within this function something like """ This algorithm for calculating number-formatting digit-count options has been extracted from InitializeNumberFormat so that it may be reused in other code. Step numbering and variable naming here is consistent with that algorithm. (Step numbering and naming will be revisited if and when this function becomes a spec formalism.) """ and then keep all the existing step-numbering comments. ::: js/src/builtin/Intl.js:1910 (Diff revision 1) > + let mnsd = options.minimumSignificantDigits; > + let mxsd = options.maximumSignificantDigits; > + > + if (mnsd !== undefined || mxsd !== undefined) { > + mnsd = GetNumberOption(options, "minimumSignificantDigits", 1, 21, 1); > + mxsd = GetNumberOption(options, "maximumSignificantDigits", mnsd, 21, 21); Hmm, this is unideal. Split up the way you have it, we have to determine whether to do significant-digit stuff here, *then* in the caller to not copy over significant-digit stuff into lazy data. That's confusing and error-prone. Fortunately, I don't think it's necessary. Instead of having this function *return* all this information, have it store it all onto an outparam, which for one caller can be |lazyNumberFormatData| and for the other can be |lazyPluralRulesData|. function CalculateDigitOptions(options, style, cDigits, lazyData) { ... } with s/lazyNumberFormatData/lazyData/g throughout the copied-over function body bits. ::: js/src/builtin/Intl.js:3035 (Diff revision 1) > } > } > > +/********** Intl.PluralRules **********/ > + > +const pluralRulesInternalProperties = { Use |var| for this. It used to be an assertion failure to use a lexical declaration at top level in self-hosted code; maybe that's gone now, not sure, but certainly it's consistent style to use |var| for this right now. ::: js/src/builtin/Intl.js:3047 (Diff revision 1) > + > + locales = intl_PluralRules_availableLocales(); > + addSpecialMissingLanguageTags(locales); > + return (this._availableLocales = locales); > + }, > + relevantExtensionKeys: [] Remove this (see later). ::: js/src/builtin/Intl.js:3063 (Diff revision 1) > + > + const r = ResolveLocale(callFunction(PluralRules.availableLocales, PluralRules), > + lazyPluralRulesData.requestedLocales, > + lazyPluralRulesData.opt, > + PluralRules.relevantExtensionKeys, > + undefined); Trailing args should be indented two more spaces. Because the last two arguments are intrinsically tied together -- |localeData| is only consulted *if* there are any |relevantExtensionKeys| -- please put them on a single line. And define a global variable |noExtensionKeys| and use that here, instead of having the empty array non-obviously defined far afield from here. ::: js/src/builtin/Intl.js:3098 (Diff revision 1) > + internalProps = resolvePluralRulesInternals(internals.lazyData); > + setInternalProperties(internals, internalProps); > + return internalProps; > +} > + > +function ResolvePlural(PluralRules, n) { Don't have this function -- just inline its contents into its single caller. ::: js/src/builtin/Intl.js:3101 (Diff revision 1) > +} > + > +function ResolvePlural(PluralRules, n) { > + const internals = getPluralRulesInternals(PluralRules, "ResolvePlural"); > + let locale = internals.locale; > + let type = internals.type; Why are you accessing these if they're never used? ::: js/src/builtin/Intl.js:3103 (Diff revision 1) > +function ResolvePlural(PluralRules, n) { > + const internals = getPluralRulesInternals(PluralRules, "ResolvePlural"); > + let locale = internals.locale; > + let type = internals.type; > + > + return intl_SelectPluralRule(PluralRules, n); |PluralRules| isn't a declared variable here. What are you actually trying to do? ::: js/src/builtin/Intl.js:3116 (Diff revision 1) > + if (isInitializedIntlObject(pluralRules)) > + ThrowTypeError(JSMSG_INTL_OBJECT_REINITED); > + > + let internals = initializeIntlObject(pluralRules); > + > + const lazyPluralRulesData = std_Object_create(null); It's unacceptable to just jump right into creating the lazy data here, without a comment explaining exactly what the lazy data will look like structurally, just as is found in the similar Initialize{Collator,{DateTime,Number}Format} functions. Splitting up Intl object logic between an immediate step and a lazy step is hairy enough. These comments are extremely useful as handy reference for knowing what data is found in these structures, when it's set/referenced in very separate locations. And the mere act of drawing one up and making sure it's correct initially, forces a certain rigor of implementation. Please add one. ::: js/src/builtin/Intl.js:3119 (Diff revision 1) > + let internals = initializeIntlObject(pluralRules); > + > + const lazyPluralRulesData = std_Object_create(null); > + > + let requestedLocales = CanonicalizeLocaleList(locales); > + lazyPluralRulesData.requestedLocales = requestedLocales; All this code in this function doesn't look implausible, but we can only say it's valid by comparing it to a spec algorithm giving a series of steps performed -- *especially* given this replicates the eager/lazy split for user-visible and user-invisible operations. Do we have one, or at least a well-fleshed-out proposal? Have we written one and deliberately made this implement it? We need one or the other here. ::: js/src/builtin/Intl.js:3158 (Diff revision 1) > + let requestedLocales = CanonicalizeLocaleList(locales); > + return SupportedLocales(availableLocales, requestedLocales, options); > +} > + > +function Intl_PluralRules_select(value) { > + let pluralRules = this; It's generally strange not to type-test |this| before proceeding to process the function arguments, where the type-test doesn't depend on the argument values. (I can only think of a single exception off the top of my head.) That's what you're doing, by performing |ToNumber| before implicitly calling |getIntlObjectInternals|. You need to add var internals = getPluralRulesInternals(this, "select"); before you call |ToNumber|, with a blank line after it to separate conceptual steps from each other. ::: js/src/builtin/Intl.js:3169 (Diff revision 1) > + var internals = getPluralRulesInternals(this, "resolvedOptions"); > + > + var result = { > + locale: internals.locale, > + type: internals.type, > + pluralCategories: internals.pluralCategories, This is pretty bad right here. |internals.pluralCategories| is an array, not a scalar value like all the rest. If you return it directly here, rather than returning a *copy* of it, any code that calls resolvedOptions() can muck with the internal data used by the PluralRules instance and generally cause badtimes to happen. You'll need to create a new array that copies over all the properties from this one. I believe |callFunction(std_Array_slice, internals.pluralCategories, 0)| should do what you need. ::: js/src/builtin/Intl.js:3178 (Diff revision 1) > + }; > + var optionalProperties = [ > + "minimumSignificantDigits", > + "maximumSignificantDigits" > + ]; > + Put this blank line above the |optionalProperties| declaration.

Comment on attachment 8806132 [details] Bug 1270146 - Add PluralRules API. https://reviewboard.mozilla.org/r/89634/#review95780 I updated the patch to your feedback, added tests and aligned it with the PluralRules proposal. My only open question is if I should aim to put it on mozIntl or Intl. It's currently Stage 3 [ https://github.com/tc39/ecma402/ ] and Chrome expressed interest in implementing it. Once we have two implementations (SpiderMonkey and V8) we'll bump it to Stage 4.

Comment on attachment 8806132 [details] Bug 1270146 - Add PluralRules API. https://reviewboard.mozilla.org/r/89634/#review89372 > Usually you wouldn't need an error-check after this -- but it seems this function *doesn't* follow the usual ICU convention of returning garbage if the incoming status is a failure. So you need to handle failure after unum_parseToUFormattable. > > That said...what's the point of this call? You use |y| only once, far below here -- and if you look at that use, it *should* be one of two uses (the other being in the second uplrules_select call), if it even makes sense to use it. But you're not using it that second time. So this doesn't make any sense. Sorry, I fixed the second use to use |y|. The reason we need this is to format the number before we send it to plural rules using the minimum/maximum*Digits options. > Hmm, this is unideal. Split up the way you have it, we have to determine whether to do significant-digit stuff here, *then* in the caller to not copy over significant-digit stuff into lazy data. That's confusing and error-prone. > > Fortunately, I don't think it's necessary. Instead of having this function *return* all this information, have it store it all onto an outparam, which for one caller can be |lazyNumberFormatData| and for the other can be |lazyPluralRulesData|. > > function CalculateDigitOptions(options, style, cDigits, lazyData) { > ... > } > > with s/lazyNumberFormatData/lazyData/g throughout the copied-over function body bits. We ended up doing something similar in the spec - we pass |intlObj|. In this patch I instead pass the |lazyData|. Hope it's ok :) > All this code in this function doesn't look implausible, but we can only say it's valid by comparing it to a spec algorithm giving a series of steps performed -- *especially* given this replicates the eager/lazy split for user-visible and user-invisible operations. Do we have one, or at least a well-fleshed-out proposal? Have we written one and deliberately made this implement it? We need one or the other here. Yeah, the spec is here: https://github.com/caridy/intl-plural-rules-spec

Comment on attachment 8806132 [details] Bug 1270146 - Add PluralRules API. https://reviewboard.mozilla.org/r/89634/#review98404 ::: js/src/builtin/Intl.cpp:1498 (Diff revision 3) > + if (!GetProperty(cx, internals, internals, cx->names().minimumSignificantDigits, > + &value)) > + { > + return nullptr; > + } > + uMinimumSignificantDigits = int32_t(value.toNumber()); `value.toInt32()` and for the other cases of this pattern in subsequent lines. ::: js/src/builtin/Intl.cpp:2841 (Diff revision 3) > + CallArgs args = CallArgsFromVp(argc, vp); > + MOZ_ASSERT(args.length() == 0); > + > + RootedValue result(cx); > + // We're going to use ULocale availableLocales as per ICU recommendation: > + // [http://bugs.icu-project.org/trac/ticket/12756]. Use an SSL link -- https://ssl.icu-project.org/trac/ticket/12756 -- without brackets around it. (Brackets are valid characters in URLs, so they shouldn't be used as delimiters.) ::: js/src/builtin/Intl.cpp:2896 (Diff revision 3) > + RootedString fmtNumValueString(cx, fmtNumValue.toString()); > + AutoStableStringChars stableChars(cx); > + if (!stableChars.initTwoByte(cx, fmtNumValueString)) > + return false; > + > + const UChar* UfmtNumValue = Char16ToUChar(stableChars.twoByteRange().begin().get()); Lowercase the "U" -- capitalized variable names are going to look like types and it'll be confusing. ::: js/src/builtin/Intl.cpp:2901 (Diff revision 3) > + const UChar* UfmtNumValue = Char16ToUChar(stableChars.twoByteRange().begin().get()); > + > + UErrorCode status = U_ZERO_ERROR; > + > + UFormattable* fmt = unum_parseToUFormattable(nf, nullptr, UfmtNumValue, > + stableChars.twoByteRange().length(), 0, &status); Align "stable" underneath "nf". ::: js/src/builtin/Intl.cpp:2994 (Diff revision 3) > + } > + > + ScopedICUObject<UPluralRules, uplrules_close> closePluralRules(pr); > + > + // We should get a C API for that in ICU 59 and switch to it > + // http://bugs.icu-project.org/trac/ticket/12772 Cite the SSL URL for this. ::: js/src/builtin/Intl.cpp:3011 (Diff revision 3) > + > + RootedObject res(cx, NewDenseEmptyArray(cx)); > + if (!res) > + return false; > + > + RootedString val(cx); This variable is unused and should be removed. ::: js/src/builtin/Intl.cpp:3018 (Diff revision 3) > + uint32_t i = 0; > + int32_t catSize; > + const char* cat; > + > + while ((cat = uenum_next(ue, &catSize, &status))) { > + if (U_FAILURE(status)) { It's not kosher to test the return value before testing the status outparam -- or at least, it's a bad habit to pick up, because its validity is API-by-API, rather than being the usual categorical "check status before doing anything else". Write this this way instead: do { cat = uenum_next(ue, &catSize, &status); if (U_FAILURE(status)) { ...fail... return false; } if (!cat) break; ...rest of the loop... } while (true); ::: js/src/builtin/Intl.cpp:3020 (Diff revision 3) > + const char* cat; > + > + while ((cat = uenum_next(ue, &catSize, &status))) { > + if (U_FAILURE(status)) { > + JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_INTERNAL_INTL_ERROR); > + return false; There appear to be some tab characters in these lines. ::: js/src/builtin/Intl.js:1893 (Diff revision 3) > +/** > + * Applies digit options used for number formatting onto the intl object. > + * > + * Spec: ECMAScript Internationalization API Specification, 11.1.1. > + */ > +function SetNumberFormatDigitOptions(lazyIntlObjData, options, mnfdDefault) { Could you name this argument just `lazyData`? This is in a file that deals only with Intl stuff, so "Intl object" is basically redundant. It may be just duplicating the spec name -- but given we're not precisely duplicating the spec algorithm because of laziness, I think we can justify a different name. ::: js/src/builtin/Intl.js:1894 (Diff revision 3) > + * Applies digit options used for number formatting onto the intl object. > + * > + * Spec: ECMAScript Internationalization API Specification, 11.1.1. > + */ > +function SetNumberFormatDigitOptions(lazyIntlObjData, options, mnfdDefault) { > + // We skip Step 1 because we set the properties on lazyData object. On *a* lazyData object? ::: js/src/builtin/Intl.js:1897 (Diff revision 3) > + */ > +function SetNumberFormatDigitOptions(lazyIntlObjData, options, mnfdDefault) { > + // We skip Step 1 because we set the properties on lazyData object. > + > + // Step 2. > + assert(typeof options === "object", "SetNumberFormatDigitOptions"); Use `IsObject(options)`. This is observably wrong as it is -- in a debug shell build, try running new Intl.NumberFormat("en-US", objectEmulatingUndefined()); or in a debug browser, try running new Intl.NumberFormat("en-US", document.all); Either testcase will assert. Please add the former as a shell test. ::: js/src/builtin/Intl.js:1918 (Diff revision 3) > + // Step 11. > + if (mnsd !== undefined || mxsd !== undefined) { > + mnsd = GetNumberOption(options, "minimumSignificantDigits", 1, 21, 1); > + mxsd = GetNumberOption(options, "maximumSignificantDigits", mnsd, 21, 21); > + lazyIntlObjData.minimumSignificantDigits = mnsd; > + lazyIntlObjData.maximumSignificantDigits = mxsd; Indentation of these four lines looks way off. Might be some tab characters in some of them, too. ::: js/src/builtin/Intl.js:2028 (Diff revision 3) > - > - // Steps 31-32. > - var mnsd = options.minimumSignificantDigits; > - var mxsd = options.maximumSignificantDigits; > > - // Step 33. > + // Step 24. At least in the latest spec I have, these step numbers don't seem to match up. SNFDO is step 24, the maximumFractionDigits bit is step 25, and grouping is steps 26-27. (Step 35, on the other hand, looks like the right number.) ::: js/src/builtin/Intl.js:2030 (Diff revision 3) > - var mnsd = options.minimumSignificantDigits; > - var mxsd = options.maximumSignificantDigits; > > - // Step 33. > - if (mnsd !== undefined || mxsd !== undefined) { > - mnsd = GetNumberOption(options, "minimumSignificantDigits", 1, 21, 1); > + // Step 24. > + if (lazyNumberFormatData.maximumFractionDigits === undefined) { > + let mxfdDefault = s === "currency" ? cDigits : s === "percent" ? 0 : 3; Use this formatting: let mxfd = s === "currency" ? cDigits : s === "percent" ? 0 : 3; Additionally, the contents of this block are only indented two spaces, when they should instead be indented four spaces. ::: js/src/builtin/Intl.js:3055 (Diff revision 3) > + return (this._availableLocales = locales); > + } > +}; > + > +/** > + * Compute an internal properties object from |lazyPluralRulestData|. s/Rulest/Rules/ ::: js/src/builtin/Intl.js:3149 (Diff revision 3) > + // > + // // optional > + // minimumSignificantDigits: integer ∈ [1, 21], > + // maximumSignificantDigits: integer ∈ [1, 21], > + // } > + // Misindented. ::: js/src/builtin/Intl.js:3169 (Diff revision 3) > + > + // Step 6 > + const type = GetOption(options, "type", "string", ["cardinal", "ordinal"], "cardinal"); > + > + // Step 7. > + lazyPluralRulesData.type = type; Just group these two: // Steps 6-7. const type = GetOption(options, "type", "string", ["cardinal", "ordinal"], "cardinal"); lazyPluralRulesData.type = type; Same as happens with steps 9-10, really -- this is a small enough bit of functionality it's better off combined. ::: js/src/tests/Intl/PluralRules/supportedLocalesOf.js:371 (Diff revision 3) > + "zh-Hant-TW", > +]; > + > +var count = Intl.PluralRules.supportedLocalesOf(locales).length; > + > +reportCompare(locales.length, count, "Number of supported locales in Intl.NumberFormat"); Why isn't this comparing all the elements of the array? Just testing count seems a bit weird, if this test (presumably) exists to make evident when an ICU upgrade changes the set of locales supporting plural rules information.

Thanks! Applied the review feedback and re-requesting r?

Comment on attachment 8806132 [details] Bug 1270146 - Add PluralRules API. https://reviewboard.mozilla.org/r/89634/#review99596 ::: js/src/builtin/Intl.cpp:1613 (Diff revision 4) > + if (!GetProperty(cx, internals, internals, cx->names().minimumSignificantDigits, > + &value)) > + { > + return nullptr; > + } > + uMinimumSignificantDigits = int32_t(value.toInt32()); `value.toInt32()` *is* an int32_t, so it's certainly not right to redundantly cast to that, anywhere. Here, you can just do uMinimumSignificantDigits = value.toInt32(); because the target is also int32_t. Same for `uMaximumSignificantDigits`. ::: js/src/builtin/Intl.cpp:1627 (Diff revision 4) > + if (!GetProperty(cx, internals, internals, cx->names().minimumIntegerDigits, > + &value)) > + { > + return nullptr; > + } > + uMinimumIntegerDigits = int32_t(value.toInt32()); For min/max integer/fraction digits, on the other hand, these variables are uint32_t. So you should do uMinimumIntegerDigits = AssertedCast<uint32_t>(value.toInt32()); to perform a checked cast to uint32_t, and same for the other two variables in this section. This will require adding #include "mozilla/Casting.h" immediately above "#include "mozilla/PodOperations.h"" near the top of the file, and it'll require adding using mozilla::AssertedCast; just above "using mozilla::IsFinite;" a little below that. ::: js/src/builtin/Intl.cpp:1749 (Diff revision 4) > if (!GetProperty(cx, internals, internals, cx->names().minimumSignificantDigits, > &value)) > { > return nullptr; > } > - uMinimumSignificantDigits = int32_t(value.toNumber()); > + uMinimumSignificantDigits = int32_t(value.toInt32()); All these assignments in this function should also be modified, similar to the comments on the assignments in the plural-rules-related bits. ::: js/src/builtin/Intl.js:1956 (Diff revision 4) > // Steps 7-8. > var matcher = GetOption(options, "localeMatcher", "string", ["lookup", "best fit"], "best fit"); > opt.localeMatcher = matcher; > > // Compute formatting options. > - // Step 15. > + // Step 14-15. The style seems to be that effectful steps get cited by number here, and the non-effectful steps simply recording the results of those steps, get cited in the resolving function. So this should be step 14 only. ::: js/src/builtin/Intl.js:1975 (Diff revision 4) > c = toASCIIUpperCase(c); > lazyNumberFormatData.currency = c; > cDigits = CurrencyDigits(c); > } > > - // Step 21. > + // Step 20-21. Just step 20. ::: js/src/builtin/Intl.js:1980 (Diff revision 4) > - // Step 21. > + // Step 20-21. > var cd = GetOption(options, "currencyDisplay", "string", ["code", "symbol", "name"], "symbol"); > if (s === "currency") > lazyNumberFormatData.currencyDisplay = cd; > > - // Step 23. > + // Step 22-24. Steps ::: js/src/builtin/Intl.js:1986 (Diff revision 4) > - lazyNumberFormatData.minimumFractionDigits = mnfd; > > - // Steps 28-29. > - var mxfdDefault; > - if (s === "currency") > - mxfdDefault = std_Math_max(mnfd, cDigits); > + // Step 25. > + if (lazyNumberFormatData.maximumFractionDigits === undefined) { > + let mxfdDefault = s === "currency" > + ? cDigits All the ? and : lines should be indented two more spaces -- those characters should be under the "s". (I like this style better, actually, but it's not what SpiderMonkey uses.) ::: js/src/builtin/Intl.js:1994 (Diff revision 4) > - mxsd = GetNumberOption(options, "maximumSignificantDigits", mnsd, 21, 21); > - lazyNumberFormatData.minimumSignificantDigits = mnsd; > - lazyNumberFormatData.maximumSignificantDigits = mxsd; > } > > - // Step 34. > + // Steps 26-27. Step 26. ::: js/src/builtin/Intl.js:1998 (Diff revision 4) > > - // Step 34. > + // Steps 26-27. > var g = GetOption(options, "useGrouping", "boolean", undefined, true); > lazyNumberFormatData.useGrouping = g; > > - // Step 43. > + // Step 36. Steps 35-36. (I'm not entirely sure why I didn't have the prior step in this comment, before.) ::: js/src/builtin/Intl.js:3124 (Diff revision 4) > + if (options === undefined) > + options = {}; > + else > + options = ToObject(options); > + > + // Step 6-7. Just Step 6, per other comments.

So, saying that I enjoy growing our ICU mock inside Intl.cpp may become my new favorite thing to do when I want to be sarcastic. Now it compiles for Android, and your feedback has been applied.

Oh, also, the inter-diff *seems* to be working when you select 4-6 which should give you the exact last update. Yay!

So, I had to land a small fix for two tests: 1) For some reason the test for supportedLanguages failed for 'en-US-POSIX' on all test machines, but *not* on my local machine. Not sure why. I switched it to 'en-US-posix' and that works both locally and on tests 2) I had to guard the test for objectEmulatingUndefined because the test is run in xpcshell and jsreftests, and the function is available only in the former. What's remaining is just this small, tiny thing - all windows builds fail. And I don't have windows to figure out why. The error log looks like this: z:/build/build/src/js/src/builtin/Intl.cpp(3233): fatal error C1001: An internal error has occurred in the compiler. mozmake.EXE[5]: *** [Unified_cpp_js_src0.obj] Error 2 The only idea that comes to my mind is that maybe windows compiler doesn't understand namespaces and icu::PluralRules gets confused with PluralRules? :waldo, any ideas?

K'boom. Just 2 hours later and I found the reason Windows machines were failing. The `using icu::PluralRules;` confused the compiler. Removing it fixes the problem and now we seem to have a clean build! :)

Comment on attachment 8806132 [details] Bug 1270146 - Add PluralRules API. https://reviewboard.mozilla.org/r/89634/#review100014 ::: js/src/tests/Intl/NumberFormat/options-emulate-undefined.js:6 (Diff revisions 4 - 8) > // |reftest| skip-if(!this.hasOwnProperty("Intl")) > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +// objectEmulatingUndefined is only available in xpcshell, not in jsreftest Technically it's available in JS shell tests that run *in the shell* but not available if they're running in the browser. Maybe "is only available when running tests in the shell, not the browser" instead. Just fix this when landing, I don't need to re-review anything. ::: js/src/tests/Intl/PluralRules/supportedLocalesOf.js:142 (Diff revisions 4 - 8) > "en-TT", > "en-TZ", > "en-UG", > "en-UM", > "en-US", > - "en-US-POSIX", > + "en-US-posix", I'm actually not sure off the top of my head that en-US-posix is supposed to be a locale, but we can roll with this to start. At best, excluding this somehow is a followup bug.

Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/80234a27490b Add PluralRules API. r=Waldo

> I'm actually not sure off the top of my head that en-US-posix is supposed to be a locale, but we can roll with this to start. It does resolve in Intl.getCanonicalLocales, but it's not available in uloc_getLocales. Seems like resolvedOptions returns it because it does have *something* matching for it, even if it's just 'en-US' (which it is). Landed! Thanks a lot for your help :waldo. Hope it'll stick :)