See https://github.com/tc39/ecma402/pull/84 and https://github.com/tc39/ecma402/issues/57 .

This is only code cleanup to fix a few things I've noticed while working on the actual changes. - Intl.cpp: Removed boilerplate code when calling self-hosted functions. - Intl.cpp: Removed a misleading assertion in CreatePluralRulesPrototype (createBlankPrototype() creates a fresh object, so none of the internal can be set already; didn't notice this when reviewing the change). - Intl.js: Added a direct call to the maybeInternalProperties() helper in getInternals() (and reordered the type checks in alphabetical order). - Intl.js: Added step comments to Intl_getCanonicalLocales and renamed the variables to match the spec (well, more or less - I've expanded "ll" to the more expressive "localeList"). - Intl.js: Reindented and added comments to Intl_getCalendarInfo. - And fixed a few code style issues (e.g. adding additional blank lines per the current code style in Intl.{cpp,h,js}...).

This is needed for a later patch in this series. I've copied the style to create NativeObject subclasses from builtin/MapObject.h, I hope that's the correct way to do this nowadays. (There are a few lines exceeding 99 chars, this will be fixed in a later patch.)

It's not possible to initialize arbitrary objects as Intl.PluralRules, and Intl.PluralRules won't use the legacy constructor semantics. Add more assertions that PluralRules methods can only be called with proper PluralRules instances and add tests for calling Intl.PluralRules as a function.

Similar to part 3, but this time for Intl.Collator.

Add Intl.[[FallbackSymbol]] as a global variable to the self-hosted environment. This change is needed for part 6 and part 7.

Implement the legacy constructor semantics for Intl.NumberFormat. There are still a few bugs in the proposed spec change, I've added FIXME notes to mark them. I'll update this patch as soon as the spec PR has been fixed.

Similar to part 6, but this time for Intl.DateTimeFormat.

And now we can finally remove the internalsMap WeakMap from Intl.js and instead store the internals objects in internal slots.

Comment on attachment 8824547 [details] [diff] [review] bug1328386-part1.patch Review of attachment 8824547 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/Intl.js @@ +3255,5 @@ > } > > +/** > + * This function is a custom method designed after Intl API, but currently > + * not part of the spec or spec proposal. """ This function is a custom function in the style of the standard Intl.* functions, that isn't part of any spec or proposal yet. """ @@ +3295,5 @@ > + localeOpt.localeMatcher = "best fit"; > + > + // 5. Let r be ResolveLocale(%DateTimeFormat%.[[availableLocales]], > + // requestedLocales, localeOpt, > + // %DateTimeFormat%.[[relevantExtensionKeys]], localeData). Align the trailing lines, please. @@ +3364,2 @@ > const localeOpt = new Record(); > // 6. Set localeOpt.[[localeMatcher]] to "best fit". Add a blank line before this, or put the two comments adjacent with the two lines of code immediately after, don't care which.

Comment on attachment 8824550 [details] [diff] [review] bug1328386-part2.patch Review of attachment 8824550 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/Intl.cpp @@ +1019,5 @@ > if (!proto) > return false; > } > > + obj = NewObjectWithGivenProto<CollatorObject>(cx, proto); Maybe assign to an |auto*| here, then assign that to |obj| as final step in the block, so you don't need the as-cast to set the reserved slot. @@ +1066,5 @@ > // brokenness in object allocation code. For the moment, hack around it by > // explicitly guarding against the possibility of the reserved slot not > // containing a private. See bug 949220. > + CollatorObject* collator = &obj->as<CollatorObject>(); > + const Value& slot = collator->getReservedSlot(CollatorObject::UCollatorSlot); I'd probably just have a one-liner like before. And same for the others. ::: js/src/builtin/Intl.h @@ +186,5 @@ > +{ > + public: > + static const Class class_; > + > + enum { UCollatorSlot, SlotCount }; Make these static constexpr uint32_t, for clearer typing and consistency with how we specify slot numbers/limits everywhere else. Same for the other classes.

Comment on attachment 8824552 [details] [diff] [review] bug1328386-part3.patch Review of attachment 8824552 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/Intl.js @@ +3199,2 @@ > // Step 2. > + if (!(IsObject(pluralRules) && IsPluralRules(pluralRules))) Would prefer |if (!IsObject || !IsPluralRules)|, myself (and later). Fewer harder-to-read parentheses nestings. ::: js/src/tests/Intl/PluralRules/call.js @@ +41,5 @@ > + ])), > + ]; > +} > + > +// Invoking [[Call]] for Intl.PluralRules always returns a new PluralRules instance. Huh, I'd have expected them to only be constructible like Uint8Array et al. are.

Comment on attachment 8824553 [details] [diff] [review] bug1328386-part4.patch Review of attachment 8824553 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/Intl.cpp @@ +993,4 @@ > } > > + Rooted<CollatorObject*> collator(cx); > + collator = NewObjectWithGivenProto<CollatorObject>(cx, proto); These don't fit in one line in 99ch? @@ +1023,5 @@ > CallArgs args = CallArgsFromVp(argc, vp); > MOZ_ASSERT(args.length() == 2); > MOZ_ASSERT(!args.isConstructing()); > // intl_Collator is an intrinsic for self-hosted JavaScript, so it cannot > // be used with "new", but it still has to be treated as a constructor. Probably can just remove this comment, now that calling Collator acts as if you constructed it. ::: js/src/builtin/Intl.js @@ +1690,5 @@ > * Spec: ECMAScript Internationalization API Specification, 10.3.2. > */ > function Intl_Collator_compare_get() { > // Check "this Collator object" per introduction of section 10.3. > + if (!(IsObject(this) && IsCollator(this))) Same |if (!IsObject || !IsCollator)| thing. (And for any subsequent patches with the similar idea, and elsewhere in this/the other patches.)

Comment on attachment 8824558 [details] [diff] [review] bug1328386-part5.patch Review of attachment 8824558 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/Intl.js @@ +1210,5 @@ > + */ > +function intlFallbackSymbol() { > + var fallbackSymbol = intlFallbackSymbolHolder.value; > + if (!fallbackSymbol) > + intlFallbackSymbolHolder.value = fallbackSymbol = std_Symbol(); Hmm. Is there a reason this symbol doesn't have a description in the spec? Seems like it wouldn't hurt for the spec to give it one. This probably could stand to be a well-defined symbol (albeit without a corresponding Symbol.* property). ::: js/src/builtin/Utilities.js @@ +42,5 @@ > // > // Do not create an alias to a self-hosted builtin, otherwise it will be cloned > // twice. > // > +// Symbol is a bare constructor without properties or methods. Bare function? Or can this be constructed? (There was that comment in the other patch about not new-ing intrinsics, the background of which I forget now.)

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10) > ::: js/src/builtin/Intl.h > @@ +186,5 @@ > > +{ > > + public: > > + static const Class class_; > > + > > + enum { UCollatorSlot, SlotCount }; > > Make these static constexpr uint32_t, for clearer typing and consistency > with how we specify slot numbers/limits everywhere else. Same for the other > classes. I thought using enums was still the way to go, because that's currently used in ModuleObject.h, MapObject.h, and Promise.{h,cpp}.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11) > ::: js/src/tests/Intl/PluralRules/call.js > @@ +41,5 @@ > > + ])), > > + ]; > > +} > > + > > +// Invoking [[Call]] for Intl.PluralRules always returns a new PluralRules instance. > > Huh, I'd have expected them to only be constructible like Uint8Array et al. > are. Maybe :gandalf knows why Intl.PluralRules is allowed to be called as a function.

Comment on attachment 8824559 [details] [diff] [review] bug1328386-part6.patch Review of attachment 8824559 [details] [diff] [review]: ----------------------------------------------------------------- My eyes are glazing over reading these tests. :-\ ::: js/src/tests/Intl/NumberFormat/call.js @@ +39,5 @@ > + ])), > + ]; > +} > + > +const intlFallbackSymbol = Object.getOwnPropertySymbols(Intl.NumberFormat.call(Object.create(Intl.NumberFormat.prototype)))[0]; lol

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13) > ::: js/src/builtin/Intl.js > @@ +1210,5 @@ > > + */ > > +function intlFallbackSymbol() { > > + var fallbackSymbol = intlFallbackSymbolHolder.value; > > + if (!fallbackSymbol) > > + intlFallbackSymbolHolder.value = fallbackSymbol = std_Symbol(); > > Hmm. Is there a reason this symbol doesn't have a description in the spec? > Seems like it wouldn't hurt for the spec to give it one. We could probably request to change https://github.com/tc39/ecma402/pull/84 to add a description. > > This probably could stand to be a well-defined symbol (albeit without a > corresponding Symbol.* property). Well-defined symbols are shared across all Realms, but [[FallbackSymbol]] is a new symbol per each Realm. (But I don't know why littledan wants to have this per-Realm instead of shared across all Realms.)

I don't think there is any good reason. We probably copied tests from previous tests. Feel free to fix it.

Comment on attachment 8824564 [details] [diff] [review] bug1328386-part8.patch Review of attachment 8824564 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/Intl.h @@ +190,5 @@ > > + enum { InternalsSlot, UCollatorSlot, SlotCount }; > + > + static_assert(InternalsSlot == INTL_INTERNALS_OBJECT_SLOT, > + "InternalsSlot must match self-hosting define for internals object slot."); Assertion messages are usually treated as sentence fragments, so no period. And for the others. ::: js/src/builtin/Intl.js @@ +1223,5 @@ > assert(IsObject(obj), "Non-object passed to initializeIntlObject"); > + assert((type === "Collator" && IsCollator(obj)) > + || (type === "DateTimeFormat" && IsDateTimeFormat(obj)) > + || (type === "NumberFormat" && IsNumberFormat(obj)) > + || (type === "PluralRules" && IsPluralRules(obj)), || at the end of the various lines.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13) > ::: js/src/builtin/Utilities.js > @@ +42,5 @@ > > // > > // Do not create an alias to a self-hosted builtin, otherwise it will be cloned > > // twice. > > // > > +// Symbol is a bare constructor without properties or methods. > > Bare function? Or can this be constructed? Yes and no. It's a constructor which always throws a TypeError when called with `new`. > (There was that comment in the > other patch about not new-ing intrinsics, the background of which I forget > now.) intl_DateTimeFormat is exported to the self-hosting global as a function, whereas std_Symbol, just like std_WeakMap, is exported as a constructor without properties. If you prefer I can change this to use JS_FN for exporting Symbol to the self-hosting global.

Rebased and applied review comments. Carrying r+ from Waldo.

Rebased and applied review comments. Carrying r+ from Waldo.

Rebased and applied review comments. Carrying r+ from Waldo.

Rebased and applied review comments. Carrying r+ from Waldo.

Rebased and applied review comments. Carrying r+ from Waldo.

Rebased and applied review comments. Carrying r+ from Waldo.

Rebased and applied review comments. Carrying r+ from Waldo.

Rebased and applied review comments. Carrying r+ from Waldo.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83ad8b9e09f3739283a72a801d975fb85a81fd0a

(In reply to André Bargull from comment #20) > If you prefer I can change this to use JS_FN for > exporting Symbol to the self-hosting global. Nah, it's fine.

Filed bug 1333621 for the PluralRules ctor without new bug.

(In reply to André Bargull from comment #29) > Try: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=83ad8b9e09f3739283a72a801d975fb85a81fd0a I see a lot of SM failures in this run.

The cgc failures look like bug 1131425. @jandem Are you okay with relaxing the error message for "js/src/jit-test/tests/ion/bug913749.js" to also accept "undefined has no properties"?

(In reply to André Bargull from comment #33) > @jandem Are you okay with relaxing the error message for > "js/src/jit-test/tests/ion/bug913749.js" to also accept "undefined has no > properties"? I'd like to understand why we get the different error message here (we run this test with different JIT flags so it should not be interpreter vs Baseline vs Ion). I'll take a look in a bit.

(In reply to Jan de Mooij [:jandem] from comment #34) > I'd like to understand why we get the different error message here (we run > this test with different JIT flags so it should not be interpreter vs > Baseline vs Ion). > > I'll take a look in a bit. OK I can reproduce this with JS_GC_ZEAL=14,25 The problem is that we run in Ion and the expression decompiler bails if frameIter.isIon(). I'm not sure why this requires CGC, but it probably has something to do with discarding type information or JIT code at the right time. Looking back at bug 913749, the exact error message doesn't matter much, so rs=me to change it to accept the other one too.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/862bc2b2553e Part 1: Remove boilerplate code and add comments in Intl code. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/36ad679ec7db Part 2: Add a NativeObject subclass for each Intl object. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/16d0d9e90e25 Part 3: Ensure PluralRules methods are always called with actual PluralRules instances. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/e1415ba91dd2 Part 4: No longer allow to initialize arbitrary objects as Intl.Collator instances per ECMA-402, 2nd edition. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/985a0c1baa89 Part 5: Add Intl.[[FallbackSymbol]] to support ECMA402, 4th edition legacy constructor semantics. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/91080dad0ff8 Part 6: Implement legacy constructor semantics for Intl.NumberFormat per ECMA-402, 4th edition. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/60adc4589abc Part 7: Implement legacy constructor semantics for Intl.DateTimeFormat per ECMA-402, 4th edition. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/e15e0f265264 Part 8: Store internals object for Intl objects in internal slot instead of using a WeakMap. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/f35fe2367a4d Part 9: Relax error message in jit-test file so the test doesn't fail when the decompiler bails out. rs=jandem

https://hg.mozilla.org/mozilla-central/rev/862bc2b2553e https://hg.mozilla.org/mozilla-central/rev/36ad679ec7db https://hg.mozilla.org/mozilla-central/rev/16d0d9e90e25 https://hg.mozilla.org/mozilla-central/rev/e1415ba91dd2 https://hg.mozilla.org/mozilla-central/rev/985a0c1baa89 https://hg.mozilla.org/mozilla-central/rev/91080dad0ff8 https://hg.mozilla.org/mozilla-central/rev/60adc4589abc https://hg.mozilla.org/mozilla-central/rev/e15e0f265264 https://hg.mozilla.org/mozilla-central/rev/f35fe2367a4d

Does this need to be considered for backport?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #39) > Does this need to be considered for backport? No, this one shouldn't be backported, because there are still some open issues in the spec for this change.