Bug 209770

Summary: [ECMA-402] Intl.RelativeTimeFormat missing in WebKit
Product: WebKit Reporter: Shane Carr <sffc>
Component: JavaScriptCoreAssignee: Ross Kirsling <ross.kirsling>
Status: RESOLVED FIXED    
Severity: Normal CC: andy, annulen, cyb.ai.815, darin, ews-watchlist, gyuyoung.kim, keith_miller, mark.lam, mathias, mmaxfield, msaboff, ross.kirsling, ryuan.choi, sbarati, sergio, sffc, ticaiolima, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Shane Carr 2020-03-30 14:34:24 PDT
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.
Comment 1 Shane Carr 2020-03-30 14:40:20 PDT
Original proposal repo: https://github.com/tc39/proposal-intl-relative-time
Comment 2 Radar WebKit Bug Importer 2020-03-30 16:38:18 PDT
<rdar://problem/61079724>
Comment 3 Ross Kirsling 2020-04-18 16:57:17 PDT Comment hidden (obsolete)
Comment 4 Ross Kirsling 2020-04-18 17:01:17 PDT
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
Comment 5 Ross Kirsling 2020-04-18 17:19:33 PDT Comment hidden (obsolete)
Comment 6 Ross Kirsling 2020-04-18 19:13:48 PDT Comment hidden (obsolete)
Comment 7 Ross Kirsling 2020-04-18 22:14:01 PDT Comment hidden (obsolete)
Comment 8 Ross Kirsling 2020-04-19 00:28:10 PDT Comment hidden (obsolete)
Comment 9 Ross Kirsling 2020-04-19 12:06:31 PDT Comment hidden (obsolete)
Comment 10 Ross Kirsling 2020-04-19 14:53:08 PDT
Created attachment 396927 [details]
Patch
Comment 11 Darin Adler 2020-04-19 17:48:28 PDT
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.
Comment 12 Ross Kirsling 2020-04-19 19:39:06 PDT
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.
Comment 13 Ross Kirsling 2020-04-19 19:44:12 PDT
(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.
Comment 14 Ross Kirsling 2020-04-19 19:58:25 PDT
(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.
Comment 15 Ross Kirsling 2020-04-19 20:18:44 PDT
Created attachment 396937 [details]
Patch for landing
Comment 16 EWS 2020-04-19 22:15:40 PDT
Committed r260349: <https://trac.webkit.org/changeset/260349>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396937 [details].
Comment 17 Darin Adler 2020-04-19 22:49:09 PDT
(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.