1421453 - Extend Intl.RelativeTimeFormat to support `type` option

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, enhancement, P3)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

In the process of implementing `mozIntl.RelativeTimeFormat` wrapper on top of the Intl.RelativeTimeFormat (bug 1407240) we identified a need for a `type` option with values `numeric` and `text` that will translate into ICU calls `format` and `formatNumeric`.

This is basically an implementation of the proposal https://github.com/tc39/proposal-intl-relative-time/issues/9

Fortunately, the patch is very simple, and I was able to convince the ECMA402 to include it into the first version of the spec, rather than waiting for v2.
We also decided to turn `numeric` to be the default type.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1421453 - Extend Intl.RelativeTimeFormat to support `type` option.

Review commit: https://reviewboard.mozilla.org/r/203692/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203692/

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8932637 [details]
Bug 1421453 - Extend Intl.RelativeTimeFormat to support `type` option.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203692/diff/1-2/

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8932637 [details]
Bug 1421453 - Extend Intl.RelativeTimeFormat to support `type` option.

https://reviewboard.mozilla.org/r/203692/#review209616

Bunch of nits mostly, looks good otherwise.

::: js/src/builtin/Intl.cpp:808
(Diff revision 2)
>  }
>  
> +int32_t
> +ureldatefmt_formatNumeric(const URelativeDateTimeFormatter* reldatefmt, double offset,
> +                    URelativeDateTimeUnit unit, UChar* result, int32_t resultCapacity,
> +                    UErrorCode* status)

Align the indentation of parameters underneath "const URel...".

::: js/src/builtin/Intl.cpp:3977
(Diff revision 2)
>  }
>  
> +enum class RelativeTimeType
> +{
> +    Numeric,
> +    Text,

Add comments by these demoing what's meant: "Natural-language strings like 'yesterday' when possible, otherwise strings with numeric components as in '7 months ago'." and "Only strings with numeric components." or something like that.

::: js/src/builtin/Intl.cpp:4030
(Diff revision 2)
> +    {
> +        JSLinearString* type = value.toString()->ensureLinear(cx);
> +        if (!type)
> +            return false;
> +
> +        if (StringEqualsAscii(type, "text")) {

I kinda want to suggest micro-optimizing this by checking for length-4 versus length-7.  But that's super-sketch, of course (at least done by hand).

So leave this as-is -- just flagging this for future thought.  Maybe we can repurpose similar code (for fast identifier-matching) in frontend/GenerateReservedWords.py for this at some point.

::: js/src/builtin/Intl.cpp:4083
(Diff revision 2)
>      ScopedICUObject<URelativeDateTimeFormatter, ureldatefmt_close> closeRelativeTimeFormat(rtf);
>  
> -    JSString* str = Call(cx, [rtf, t, relDateTimeUnit](UChar* chars, int32_t size, UErrorCode* status) {
> +    JSString* str;
> +    if (relDateTimeType == RelativeTimeType::Text) {
> +        str = Call(cx, [rtf, t, relDateTimeUnit](UChar* chars, int32_t size, UErrorCode* status) {
> -        return ureldatefmt_format(rtf, t, relDateTimeUnit, chars, size, status);
> +            return ureldatefmt_format(rtf, t, relDateTimeUnit, chars, size, status);

How about instead of fully duplicating the two calls, you instead just add `relDateTimeType` to the capture-list and then make the function body be

    auto fmt = relDateTimeType == RelativeTimeType::Text
               ? ureldatefmt_format
               : ureldatefmt_formatNumeric;
    return fmt(rtf, t, relDateTimeUnit, chars, size, status);

which is shorter.  Also has the benefit of not duplicating the mess of code Call will expand to.

::: js/src/builtin/Intl.cpp:4084
(Diff revision 2)
>  
> -    JSString* str = Call(cx, [rtf, t, relDateTimeUnit](UChar* chars, int32_t size, UErrorCode* status) {
> +    JSString* str;
> +    if (relDateTimeType == RelativeTimeType::Text) {
> +        str = Call(cx, [rtf, t, relDateTimeUnit](UChar* chars, int32_t size, UErrorCode* status) {
> -        return ureldatefmt_format(rtf, t, relDateTimeUnit, chars, size, status);
> +            return ureldatefmt_format(rtf, t, relDateTimeUnit, chars, size, status);
> -    });
> +        });

Couple tab characters to remove here, please.  (Although the smaller change I mentioned earlier would probably kill these off anyway.)

::: js/src/builtin/Intl.js:3703
(Diff revision 2)
>  
>      // Steps 13-14.
>      const style = GetOption(options, "style", "string", ["long", "short", "narrow"], "long");
>      lazyRelativeTimeFormatData.style = style;
>  
> +    const type = GetOption(options, "type", "string", ["numeric", "text"], "numeric");

Add a comment before this pointing at the Github issue, indicating this isn't in the spec quite yet, or something like that.  Basically I want to avoid having an un-step-number-annotated bit of code here.

::: js/src/tests/Intl/RelativeTimeFormat/format.js:71
(Diff revision 2)
> -assertEq(rtf.format(0, "hour"), "in 0 hours");
> +  assertEq(rtf.format(0, "hour"), "in 0 hours");
> -assertEq(rtf.format(-0, "hour"), "in 0 hours");
> +  assertEq(rtf.format(-0, "hour"), "in 0 hours");
> -assertEq(rtf.format(-1, "hour"), "1 hour ago");
> +  assertEq(rtf.format(-1, "hour"), "1 hour ago");
> -assertEq(rtf.format(1, "hour"), "in 1 hour");
> +  assertEq(rtf.format(1, "hour"), "in 1 hour");
>  
> -assertEq(rtf.format(0, "day"), "today");
> +  assertEq(rtf.format(0, "day"), "today");

All these indents look like tab-characters to kill off.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8932637 [details]
Bug 1421453 - Extend Intl.RelativeTimeFormat to support `type` option.

https://reviewboard.mozilla.org/r/203692/#review209622

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8932637 [details]
Bug 1421453 - Extend Intl.RelativeTimeFormat to support `type` option.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203692/diff/2-3/

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

> Couple tab characters to remove here, please.
(...)
> All these indents look like tab-characters to kill off.


Those are not tab chars, the indentation changed in both cases - in the former due to call structure change (which changed back in result of your feedback), and in the latter I captured the tests in a block for `text` vs `numeric`.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27ce27ac8b1d
Extend Intl.RelativeTimeFormat to support `type` option. r=Waldo

Comment 8

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/27ce27ac8b1d