Bug 209770 – [ECMA-402] Intl.RelativeTimeFormat missing in WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Shane Carr]

Intl.RelativeTimeFormat has reached Stage 4 in TC39 as of December 2019, and the other major browser engines (V8 and SpiderMonkey) have been shipping it for some time.

As usage of Intl.RelativeTimeFormat increases throughout the ecosystem, WebKit users will be left behind with legacy polyfills, leading to poorer performance relative to other browsers.  At Google, we are currently weighing our options for calling Intl.RelativeTimeFormat in supported environments in order to give users better performance and smaller download sizes.

ICU4C exposes C and C++ APIs that can be used to implement Intl.RelativeTimeFormat.  Implementing Intl.RelativeTimeFormat is largely a matter of adding the glue between JavaScript and ICU4C, as WebKit already does for other Intl types.

[Shane Carr]

Original proposal repo: https://github.com/tc39/proposal-intl-relative-time

[Radar WebKit Bug Importer]

<rdar://problem/61079724>

[Ross Kirsling]

Created attachment 396874 [details]
Patch

[Ross Kirsling]

Comment on attachment 396874 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396874&action=review

Notes!

1. I'll need to rebase this on top of bug 202599 once it's landed.

> Source/JavaScriptCore/ChangeLog:85
> +        Perform three corrections for Intl classes:
> +          1. Collator should be the only class with unique "available locales".
> +             [unum|udat]_getAvailable exist but they've deferred to uloc_getAvailable for 20 years.
> +          2. isUnicodeLocaleIdentifierType isn't just `alphanum{3,8}` but rather `alphanum{3,8} (sep alphanum{3,8})*`.
> +             This is my own mistake from r239941.
> +          3. supportedLocalesOf entries should not be frozen.
> +             Changed in https://github.com/tc39/ecma402/pull/278.

2. I'm happy to split these out from this patch, but they're all pretty small changes and were all detected by test262 cases for RelativeTimeFormat.

> JSTests/ChangeLog:16
> +        Tests for Polish appear to be wrong and should be corrected in test262.

3. V8 and SM are ignoring minimumGroupingSeparators for RelativeTimeFormat but not for NumberFormat.
   This seems completely bogus, I've reported it here: https://github.com/tc39/test262/pull/1669#issuecomment-615534315

[Ross Kirsling]

Created attachment 396876 [details]
Patch

[Ross Kirsling]

Weird, apparently pluralization doesn't work in ICU 62?

> Exception: Error: expected in 1 second but got in 1 seconds
> [e-mail]:5:24
> global [e-mail]:170:13

[Ross Kirsling]

Created attachment 396892 [details]
Patch

[Ross Kirsling]

Created attachment 396896 [details]
Patch

[Ross Kirsling]

Created attachment 396922 [details]
Patch

[Ross Kirsling]

Created attachment 396927 [details]
Patch

[Darin Adler]

Comment on attachment 396927 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396927&action=review

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:507
> +void IntlNumberFormat::formatToPartsInternal(JSGlobalObject* globalObject, double value, const String& formatted, UFieldPositionIterator* iterator, JSArray* parts, JSString* unit)

Not sure why we never do this in JavaScriptCore: Seems the global object pointer should be a reference, since it can’t be null. Same for UFieldPositionIterator* and JSArray*.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:516
> +    Vector<IntlNumberFormatField> fields(stringLength, literalField);

Can avoid allocating memory on the heap if we use inline capacity in this vector. Not sure what the performance characteristics of this are.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:531
> +    auto unitName = Identifier::fromString(vm, "unit");

Just moved code, but seems a little wasteful to do this operation when unit is nullptr.

> Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.cpp:40
> +static const char* const relevantExtensionKeys[1] = { "nu" };

In new code we should use constexpr for this instead.

    constexpr const char* relevantExtensionKeys[1] = { "nu" };

Obviates the need for static and const, and compatibele with some additional special stuff.

> Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.cpp:63
> +    IntlRelativeTimeFormat* format = new (NotNull, allocateCell<IntlRelativeTimeFormat>(vm.heap)) IntlRelativeTimeFormat(vm, structure);

auto

> Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.cpp:86
> +    IntlRelativeTimeFormat* thisObject = jsCast<IntlRelativeTimeFormat*>(cell);

auto

> Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.cpp:92
> +namespace IntlRTFInternal {

Not sure all these things need to be wrapped in namespaces like this.

> Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.cpp:207
> +    return unit.endsWith("s") ? unit.left(unit.length() - 1) : unit;

endsWith('s') is more efficient; I think I added it recently

> Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.h:76
> +    struct UFieldPositionIteratorDeleter {
> +        void operator()(UFieldPositionIterator*) const;
> +    };

This one isn’t used in the header; maybe it doesn’t need to be a member, or maybe the definition can be in the .cpp file even if it stays a member.

> Source/JavaScriptCore/runtime/IntlRelativeTimeFormatConstructor.cpp:58
> +    IntlRelativeTimeFormatConstructor* constructor = new (NotNull, allocateCell<IntlRelativeTimeFormatConstructor>(vm.heap)) IntlRelativeTimeFormatConstructor(vm, structure);

auto

> Source/JavaScriptCore/runtime/IntlRelativeTimeFormatConstructor.cpp:115
> +    const HashSet<String>& availableLocales = intlRelativeTimeFormatAvailableLocales();

auto&

> Source/JavaScriptCore/runtime/IntlRelativeTimeFormatConstructor.cpp:117
> +    Vector<String> requestedLocales = canonicalizeLocaleList(globalObject, callFrame->argument(0));

auto

> Source/JavaScriptCore/runtime/IntlRelativeTimeFormatConstructor.h:37
> +    typedef InternalFunction Base;

Use using in new code?

> Source/JavaScriptCore/runtime/IntlRelativeTimeFormatPrototype.cpp:58
> +    IntlRelativeTimeFormatPrototype* object = new (NotNull, allocateCell<IntlRelativeTimeFormatPrototype>(vm.heap)) IntlRelativeTimeFormatPrototype(vm, structure);

auto

> Source/JavaScriptCore/runtime/IntlRelativeTimeFormatPrototype.cpp:85
> +    IntlRelativeTimeFormat* relativeTimeFormat = jsDynamicCast<IntlRelativeTimeFormat*>(vm, callFrame->thisValue());

auto

> Source/JavaScriptCore/runtime/IntlRelativeTimeFormatPrototype.cpp:104
> +    IntlRelativeTimeFormat* relativeTimeFormat = jsDynamicCast<IntlRelativeTimeFormat*>(vm, callFrame->thisValue());

auto

> Source/JavaScriptCore/runtime/IntlRelativeTimeFormatPrototype.cpp:123
> +    IntlRelativeTimeFormat* relativeTimeFormat = jsDynamicCast<IntlRelativeTimeFormat*>(vm, callFrame->thisValue());

auto

> Source/JavaScriptCore/runtime/IntlRelativeTimeFormatPrototype.h:35
> +    static constexpr unsigned StructureFlags = Base::StructureFlags | HasStaticPropertyTable;

I don’t think we need static when we have constexpr.

[Ross Kirsling]

Thanks for the review!

(In reply to Darin Adler from comment #11)
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:507
> > +void IntlNumberFormat::formatToPartsInternal(JSGlobalObject* globalObject, double value, const String& formatted, UFieldPositionIterator* iterator, JSArray* parts, JSString* unit)
> 
> Not sure why we never do this in JavaScriptCore: Seems the global object
> pointer should be a reference, since it can’t be null. Same for
> UFieldPositionIterator* and JSArray*.

It is odd but JSGlobalObject and JSArray have this convention... I will change UFieldPositionIterator* though.

> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:516
> > +    Vector<IntlNumberFormatField> fields(stringLength, literalField);
> 
> Can avoid allocating memory on the heap if we use inline capacity in this
> vector. Not sure what the performance characteristics of this are.

I guess we can do 32, since that's our initial capacity when formatting the number.

> > Source/JavaScriptCore/runtime/IntlRelativeTimeFormatPrototype.h:35
> > +    static constexpr unsigned StructureFlags = Base::StructureFlags | HasStaticPropertyTable;
> 
> I don’t think we need static when we have constexpr.

It is a static field though? Either way this too is JSC's unanimous convention.

[Ross Kirsling]

(In reply to Ross Kirsling from comment #12)
> It is odd but JSGlobalObject and JSArray have this convention... I will
> change UFieldPositionIterator* though.

On second thought, I might not change UFieldPositionIterator* either -- seems a bit odd when it's just being passed from one C API to another.

[Ross Kirsling]

(In reply to Darin Adler from comment #11)
> > Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.cpp:207
> > +    return unit.endsWith("s") ? unit.left(unit.length() - 1) : unit;
> 
> endsWith('s') is more efficient; I think I added it recently

Funny enough, we have StringView::startsWith(UChar) but not StringView::endsWith(UChar) at the moment. Happy to add it in a follow-up though.

[Ross Kirsling]

Created attachment 396937 [details]
Patch for landing

[EWS]

Committed r260349: <https://trac.webkit.org/changeset/260349>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396937 [details].

[Darin Adler]

(In reply to Ross Kirsling from comment #14)
> Funny enough, we have StringView::startsWith(UChar) but not
> StringView::endsWith(UChar) at the moment. Happy to add it in a follow-up
> though.

I was thinking of my patch in progress for bug 210431, which adds that function. Thought I had already landed it.