Closed Bug 1325106 Opened 3 years ago Closed 3 years ago

PluralRules constructor cleanup


(Core :: JavaScript: Internationalization API, defect)

Not set



Tracking Status
firefox53 --- fixed


(Reporter: anba, Assigned: anba)



(2 files, 1 obsolete file)

- We don't need to export intl_PluralRules from Intl.h [1] unless we expect it to be used from self-hosted code. 
- PluralRules shouldn't use the ecma-402 constructor logic at all [2].
- PluralRules doesn't support subclassing, cf. bug 1318017 where the other Intl constructors were updated.

Assignee: nobody → andrebargull
Attached patch bug1325106-part1.patch (obsolete) — Splinter Review
Mostly code clean-up and fixed style nits:

- Replaced explicit args.length() checks with simpler args.get(n) calls
- Directly call HasProperty with a PropertyName* in NewUNumberFormatForPluralRules and NewUNumberFormat to match the style of the following GetProperty calls
- The GetProperty calls in NewUNumberFormatForPluralRules and NewUNumberFormat fit into a single line, so we can remove the braces around if-statement body
- Intl.PluralRules isn't exported to selfhosted code, so we don't need js::intl_PluralRules and can keep the constructor in a single function
- Replaced some JSAutoByteString with JSLinearString, this saves us a string copy when constructing the JSAutoByteString (similar to bug 1321771)
- Added a missing U_FAILURE call after PluralRules#getKeywords()
- Also added some more assertions and fixed some other style nits where lines were longer than 99 chars
Attachment #8821346 - Flags: review?(jwalden+bmo)
Removed the legacy ECMA-402 constructor code from Intl.PluralRules and made Intl.PluralRules subclassable (similar to bug 1318017).
Attachment #8821348 - Flags: review?(jwalden+bmo)
Comment on attachment 8821346 [details] [diff] [review]

Review of attachment 8821346 [details] [diff] [review]:

This might moot the patch for bug 1325246 (although not necessarily the question I had about the patch already posted there).

Thanks for the cleanups -- definitely getting a bit fatigued by the end of that review process, and post-landing fixups were on a todo list somewhere.

::: js/src/builtin/Intl.cpp
@@ +1766,5 @@
>      }
>      UErrorCode status = U_ZERO_ERROR;
> +    UNumberFormat* nf = unum_open(UNUM_DECIMAL, nullptr, 0, icuLocale(locale.ptr()), nullptr,
> +                                  &status);

Mild preference for bumping the function call onto a new line, indented by four spaces:

    UNumberFormat* nf =

because then it doesn't have to wrap at all and that's more readable, but this is okay.
Attachment #8821346 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8821348 [details] [diff] [review]

Review of attachment 8821348 [details] [diff] [review]:

::: js/src/tests/Intl/PluralRules/construct-newtarget.js
@@ +32,5 @@
> +// Set a different constructor as NewTarget.
> +obj = Reflect.construct(MyPluralRules, [], Array);
> +assertEq(obj instanceof MyPluralRules, false);
> +assertEq(obj instanceof Intl.PluralRules, false);
> +assertEq(obj instanceof Array, true);

This stuff is nuts.
Attachment #8821348 - Flags: review?(jwalden+bmo) → review+
Updated part 1 per review comments, carrying r+ from Waldo.
Attachment #8821346 - Attachment is obsolete: true
Attachment #8822179 - Flags: review+
Pushed by
Part 1: Clean-up new Intl.PluralRules code to match latest Intl.cpp code. r=Waldo
Part 2: Allow to create Intl.PluralRules subclasses and remove ECMA402 legacy constructor logic for Intl.PluralRules. r=Waldo
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.