[webkit-reviews] review granted: [Bug 209770] [ECMA-402] Intl.RelativeTimeFormat missing in WebKit : [Attachment 396927] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 209770: [ECMA-402] Intl.RelativeTimeFormat missing in WebKit
https://bugs.webkit.org/show_bug.cgi?id=209770

Attachment 396927: Patch

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




--- Comment #11 from Darin Adler <darin at apple.com> ---
Comment on attachment 396927
  --> https://bugs.webkit.org/attachment.cgi?id=396927
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.