Closed Bug 1270140 Opened 8 years ago Closed 7 years ago

Add Intl.RelativeTimeFormat

Categories

(Core :: JavaScript: Internationalization API, defect)

Product:
Core
Component:
JavaScript: Internationalization API
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

(
URL
)

Details

(Keywords: dev-doc-complete, Whiteboard: [gecko-l20n])

Attachments

(1 file, 1 obsolete file)

Bug 1270140 - Add Intl.RelativeTimeFormat.
59 bytes, text/x-review-board-request
Waldo
: review+
Details 
As part of the move to L20n in Gecko, we need a set of Intl APIs that are currently being baked in ECMA 402. One of them is Intl.RelativeTimeFormat.

It uses CLDR data that we already have [0] and we may want to reuse ICU API [1]

Now, I recognize that it's early for landing this as public API, but I also feel like writing this as an internal module that depends on CLDR/ICU and will eventually be replaced by JS Intl API anyway is not optimal.

So I'd like to suggest that we write it as Intl API and expose only to chrome for now allowing us to use it for Firefox i18n.

:waldo, does it sound reasonable or would you prefer us to write it differently until we reach higher stage with the ECMA 402 proposal?

[0] https://dxr.mozilla.org/mozilla-central/source/intl/icu/source/data/locales/en.txt#1126
[1] http://icu-project.org/apiref/icu4c/classicu_1_1RelativeDateTimeFormatter.html
Blocks: 1186262
Flags: needinfo?(jwalden+bmo)
Blocks: 595812
Depends on: 1270146
No longer depends on: 1270146
I met with Waldo and we decided to target this to land in SpiderMonkey but not on Intl object for now and expose only to chrome code.

Once the spec cools down, we can expose it on Intl easily.

I'll try to write the patch now.
Flags: needinfo?(jwalden+bmo)
I updated the spec and will start working on the impl.
URL: https://github.com/caridy/intl-relati... → https://rawgit.com/zbraniecki/intl-re...
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
We need bug 1270146 for this API to work.
Depends on: 1270146
Mass change dependency tree for bug 1279002 into a whiteboard keyword.
No longer blocks: gecko-l20n
Whiteboard: [gecko-l20n]
Got to start working on it, this is a WIP.

I got stuck on a linker error that I don't understand.

When I call to `udatrelfmt_open` in Intl.cpp, the linker errors with:

(...)
    INPUT("../../config/external/icu/i18n/visibledigits.o")
    INPUT("../../config/external/icu/i18n/vtzone.o")
    INPUT("../../config/external/icu/i18n/vzone.o")
    INPUT("../../config/external/icu/i18n/windtfmt.o")
    INPUT("../../config/external/icu/i18n/winnmfmt.o")
    INPUT("../../config/external/icu/i18n/wintzimpl.o")
    INPUT("../../config/external/icu/i18n/zonemeta.o")
    INPUT("../../config/external/icu/i18n/zrule.o")
    INPUT("../../config/external/icu/i18n/ztrans.o")
    INPUT("../../config/external/icu/data/icudata.o")

../../build/unix/gold/ld: error: /home/zbraniecki/projects/mozilla/mozilla-central/js/src/build_DBG.OBJ/js/src/Unified_cpp_js_src0.o: requires dynamic R_X86_64_PC32 reloc against 'ureldatefmt_open_58' which may overflow at runtime; recompile with -fPIC
../../build/unix/gold/ld: error: read-only segment has dynamic relocations
/home/zbraniecki/projects/mozilla/mozilla-central/js/src/builtin/Intl.cpp:4146: error: undefined reference to 'ureldatefmt_open_58'
collect2: error: ld returned 1 exit status
/home/zbraniecki/projects/mozilla/mozilla-central/config/rules.mk:800: recipe for target 'libmozjs-53a1.so' failed
make[3]: *** [libmozjs-53a1.so] Error 1
make[3]: Leaving directory '/home/zbraniecki/projects/mozilla/mozilla-central/js/src/build_DBG.OBJ/js/src'
/home/zbraniecki/projects/mozilla/mozilla-central/config/recurse.mk:71: recipe for target 'js/src/target' failed
make[2]: *** [js/src/target] Error 2
make[2]: Leaving directory '/home/zbraniecki/projects/mozilla/mozilla-central/js/src/build_DBG.OBJ'
/home/zbraniecki/projects/mozilla/mozilla-central/config/recurse.mk:32: recipe for target 'compile' failed
make[1]: *** [compile] Error 2
make[1]: Leaving directory '/home/zbraniecki/projects/mozilla/mozilla-central/js/src/build_DBG.OBJ'
/home/zbraniecki/projects/mozilla/mozilla-central/config/rules.mk:523: recipe for target 'default' failed
make: *** [default] Error 2

===============

STR:

1) Apply the patch on m-c
2) Try to compile

I'm on Ubuntu 16.10, gcc 6.2.

I found a few bugs with similar error message - bug 1194520, bug 1002729, bug 860222 and a nice article that attempts to explain to me why errors like this are displayed: https://www.collabora.com/about-us/blog/2014/10/01/dynamic-relocs,-runtime-overflows-and-fpic/

but unfortunately, that goes way above my understanding of C++ and linking :(

:waldo - help pls
Flags: needinfo?(jwalden+bmo)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #7)
> I got stuck on a linker error that I don't understand.
> 

ureldatefmt.h requires break iterator support [1], which we disable in [2] to save space. But it seems like break iterators are only needed for UDISPCTX_CAPITALIZATION_FOR_BEGINNING_OF_SENTENCE [3], so you may be able to change "config/external/icu/defs.mozbuild" without actually recompiling the ICU data file to include break iterator data. (Unless you want to support UDISPCTX_CAPITALIZATION_FOR_BEGINNING_OF_SENTENCE, in that case you need to recompile the ICU data file with break iterator data [4].) 

TLDR: 
Remove the "UCONFIG_NO_BREAK_ITERATION = True," line from config/external/icu/defs.mozbuild [2] and then do the usual autoconf, configure, and make steps to recompile everything from scratch.


[1] https://dxr.mozilla.org/mozilla-central/rev/ac3275723df59db0f09198fdb61b51e7002c391a/intl/icu/source/i18n/unicode/ureldatefmt.h#15
[2] https://dxr.mozilla.org/mozilla-central/rev/ac3275723df59db0f09198fdb61b51e7002c391a/config/external/icu/defs.mozbuild#18
[3] https://dxr.mozilla.org/mozilla-central/rev/ac3275723df59db0f09198fdb61b51e7002c391a/intl/icu/source/i18n/reldatefmt.cpp#721
[4] https://dxr.mozilla.org/mozilla-central/rev/ac3275723df59db0f09198fdb61b51e7002c391a/intl/update-icu.sh#36
Flags: needinfo?(jwalden+bmo)
> Remove the "UCONFIG_NO_BREAK_ITERATION = True," line from config/external/icu/defs.mozbuild [2] and then do the usual autoconf, configure, and make steps to recompile everything from scratch.

Worked like a charm! Thanks Andre :)
I finished the core of the patch and now will work on two things I need to finish before I request review for this:

1) tests
2) update spec text and document the code

I filed issues that I noticed while working on the implementation in the spec repo: https://github.com/caridy/intl-relative-time-spec/issues

I made several decisions that are quite arbitrary, so I'll document them in case anyone interested will want to counter them:

a) I decided not to use UDisplayContext for now. That means that this formatter is currently not suitable for use inside localization strings.

I decided to do this in order to reduce the scope of the patch and get it landed quicker. In a follow-up I will want to add UDisplayContext, and I believe that it'll require us to rebuild the ICU database.

b) I decided not to include date+time combos. That means that strings like "Tomorrow at 3:45pm" will not be possible yet.

This is also in order to reduce the scope of the patch. In the follow up I'll propose an extension to allow for that. (as per https://github.com/caridy/intl-relative-time-spec/issues/10 )

Also, this API is fairly early in the standardization process. My current estimation is that we can aim to push for stage4 by the end of this year.
In that aspect, it's different than previous APIs (PluralRules, formatToParts), and closer to `getDisplayNames`, `getCalendarInfo` and `getLocaleInfo`.
Attachment #8826838 - Flags: review?(jwalden+bmo)
Comment on attachment 8826838 [details]
Bug 1270140 - Add mozIntl.RelativeTimeFormat.

https://reviewboard.mozilla.org/r/104710/#review118038

Some of the reviewing bits in here are a little cursory (e.g. all aspects of the tests, some of the C++ I skimmed for plausibility, etc.), once I concluded this wasn't ready.  But things that looked obviously off, I commented on if I saw them.

::: js/src/builtin/Intl.cpp:4024
(Diff revision 5)
> +    // Step 1 (Handled by OrdinaryCreateFromConstructor fallback code).
> +
> +    // Step 2 (Inlined 9.1.14, OrdinaryCreateFromConstructor).
> +    RootedObject proto(cx);
> +    if (args.isConstructing() && !GetPrototypeFromCallableConstructor(cx, args, &proto))
> +        return false;

Shouldn't this only be a constructor, now?  That seems the general tack things are taking now.

::: js/src/builtin/Intl.cpp:4043
(Diff revision 5)
> +    RootedValue locales(cx, args.get(0));
> +    RootedValue options(cx, args.get(1));
> +
> +    // Step 3.
> +    if (!IntlInitialize(cx, obj, cx->names().InitializeRelativeTimeFormat, locales, options))
> +        return false;

I think IntlInitialize got changed recently to deal with some weird subclassing legacy things, so this needs updating probably.

::: js/src/builtin/Intl.cpp:4073
(Diff revision 5)
> +    RootedFunction ctor(cx);
> +    ctor = global->createConstructor(cx, &RelativeTimeFormat, cx->names().RelativeTimeFormat, 0);
> +    if (!ctor)
> +        return nullptr;
> +
> +    RootedNativeObject proto(cx, global->createBlankPrototype(cx, &RelativeTimeFormatClass));

I somewhat doubt we want this prototype object to be an actual RelativeTimeFormat object, either.

::: js/src/builtin/Intl.cpp:4161
(Diff revision 5)
> + * We also combine ComputeTimeUnits (1.1.7) into this function.
> + *
> + * Spec: ECMAScript 402 API, RelativeTimeFormat, 1.1.6-7
> + */
> +URelativeDateTimeUnit
> +GetBestMatchUnit(JSContext* cx, double ms, HandleString unitStr, double& v)

`double*` v for the outparam, so it's clearer at the call site that the double will be changed.

::: js/src/builtin/Intl.cpp:4162
(Diff revision 5)
> + *
> + * Spec: ECMAScript 402 API, RelativeTimeFormat, 1.1.6-7
> + */
> +URelativeDateTimeUnit
> +GetBestMatchUnit(JSContext* cx, double ms, HandleString unitStr, double& v)
> +{

If `v` is always finite or limited in some way -- is it? -- we should assert that range limitation at start of function, seeing as we rely on all these abses and divisions to not overflow below.

...or no, you've implemented no restrictions on what `v` might be.  Seems like the spec should handle infinities and NaN specially, perhaps just by throwing.

::: js/src/builtin/Intl.cpp:4163
(Diff revision 5)
> + * Spec: ECMAScript 402 API, RelativeTimeFormat, 1.1.6-7
> + */
> +URelativeDateTimeUnit
> +GetBestMatchUnit(JSContext* cx, double ms, HandleString unitStr, double& v)
> +{
> +    RootedLinearString unit(cx, unitStr->ensureLinear(cx));

This can fail/return null -- you need to return true/false from this function overall, and the URDTU should be an outparam pointer.

::: js/src/builtin/Intl.cpp:4169
(Diff revision 5)
> +
> +    double msPerSecond = 1000;
> +    double msPerMinute = msPerSecond * 60;
> +    double msPerHour = msPerMinute * 60;
> +    double msPerDay = msPerHour * 24;
> +    double msPerWeek = msPerDay * 7;

We have constants for this stuff in "vm/DateTime.h".  Use them, don't do this.

::: js/src/builtin/Intl.cpp:4172
(Diff revision 5)
> +    double msPerHour = msPerMinute * 60;
> +    double msPerDay = msPerHour * 24;
> +    double msPerWeek = msPerDay * 7;
> +    double msPer400Years = msPerDay * 146097;
> +
> +    double rawYear = ms * 400 / msPer400Years;

msPer400Years/rawYear probably can stick in this file, tho.

::: js/src/builtin/Intl.cpp:4174
(Diff revision 5)
> +    double msPerWeek = msPerDay * 7;
> +    double msPer400Years = msPerDay * 146097;
> +
> +    double rawYear = ms * 400 / msPer400Years;
> +
> +    double seconds = round(ms / msPerSecond);

The spec shouldn't rely on (an apparently not-specified-in-ECMA-262) round.  For user-visible purposes, rounding 3.5 to 4 but 2.5 to 2 is probably not expected behavior.  Please replace this with something more desirable.

::: js/src/builtin/Intl.cpp:4183
(Diff revision 5)
> +    double weeks = round(ms / msPerWeek);
> +    double months = round(rawYear * 12);
> +    double quarters = round(rawYear * 4);
> +    double years = round(rawYear);
> +
> +    if (StringEqualsAscii(unit, "second")) {

These if-blocks end in return, so don't follow them with `else if`, just `if`.

    if (StringEqualsAscii(unit, "second")) {
        v = seconds;
        return UDAT_REL_UNIT_SECOND;
    }
    
    if (StringEqualsAscii(unit, "minute")) {
        v = minutes;
        return UDAT_REL_UNIT_MINUTE;
    }

...and so on.

That said, I'd prefer if the divisions/rounding were interleaved with these string-comparisons/returns.  There's no reason to do the umpteen rounds/divisions above, if only the one for seconds is needed.

::: js/src/builtin/Intl.cpp:4209
(Diff revision 5)
> +    } else if (StringEqualsAscii(unit, "year")) {
> +        v = years;
> +        return UDAT_REL_UNIT_YEAR;
> +    }
> +
> +    if (abs(seconds) < 60) {

Use mozilla::Abs in mozilla/MathAlgorithms.h for this purpose.

::: js/src/builtin/Intl.cpp:4260
(Diff revision 5)
> +        return false;
> +
> +    if (!GetProperty(cx, internals, internals, cx->names().unit, &value))
> +        return false;
> +    RootedString unit(cx, value.toString());
> +    if (!unit)

Assuming `value` is a string, `value.toString()` is never null.  No need for failure-check here.

::: js/src/builtin/Intl.cpp:4282
(Diff revision 5)
> +    UDateRelativeDateTimeFormatterStyle relDateTimeStyle = UDAT_STYLE_LONG;
> +    if (StringEqualsAscii(style, "short")) {
> +        relDateTimeStyle = UDAT_STYLE_SHORT;
> +    } else if (StringEqualsAscii(style, "narrow")) {
> +        relDateTimeStyle = UDAT_STYLE_NARROW;
> +    }

No braces on these.

::: js/src/builtin/Intl.cpp:4292
(Diff revision 5)
> +    // ICU doesn't handle -0 well. This worksaround the bug by turning every
> +    // `-0.0` into `0.0`.
> +    // See: http://bugs.icu-project.org/trac/ticket/12936
> +    if (v == 0.0 && signbit(v) == 1) {
> +        v = 0.0;
> +    }

Replace this with

    v += 0.0;

::: js/src/builtin/Intl.cpp:4295
(Diff revision 5)
> +    if (v == 0.0 && signbit(v) == 1) {
> +        v = 0.0;
> +    }
> +
> +    UErrorCode status = U_ZERO_ERROR;
> +    URelativeDateTimeFormatter* rtf = ureldatefmt_open(

Use this style:

    URelativeDateTimeFormatter* rtf =
        ureldatefmt_open(icuLocale(locale.ptr()), nullptr, ...,
                         ...subsequent args...);

::: js/src/builtin/Intl.cpp:4315
(Diff revision 5)
> +        return false;
> +
> +    int32_t size;
> +
> +    if (StringEqualsAscii(type, "numeric")) {
> +        size = ureldatefmt_formatNumeric(

This all should use the new to-string lambda stuff anba landed.

::: js/src/builtin/Intl.cpp:4336
(Diff revision 5)
> +                    Char16ToUChar(chars.begin()),
> +                    size,
> +                    &status);
> +        }
> +    } else {
> +        size = ureldatefmt_format(

Can you assert the `type` string has some particular value here?

::: js/src/builtin/Intl.js:3289
(Diff revision 5)
> +    //
> +    //   {
> +    //     requestedLocales: List of locales,
> +    //     type: "text" / "numeric",
> +    //     style: "long" / "short" / "narrow",
> +    //     unit: "best fit" / "second" / "minute" / "hour" / "day" / "week" / "month" / "year",

You're missing "quarter" here.

::: js/src/builtin/Intl.js:3324
(Diff revision 5)
> +
> +    // Steps 10-11.
> +    const style = GetOption(options, "style", "string", ["long", "short", "narrow"], "long");
> +    lazyRelativeTimeFormatData.style = style;
> +
> +

Stray blank line.

::: js/src/builtin/Intl.js:3327
(Diff revision 5)
> +    lazyRelativeTimeFormatData.style = style;
> +
> +
> +    // Step 12.
> +    let opt = new Record();
> +    lazyRelativeTimeFormatData.opt = opt;

I'd prefer if you created `opt` after computing the locale matcher.  Also in the spec as well.

::: js/src/tests/Intl/RelativeTimeFormat/construct-newtarget.js:10
(Diff revision 5)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +addIntlExtras(Intl);
> +
> +// Test subclassing %Intl.RelativeTimeFormat% works correctly.
> +class MyRelativeTimeFormat extends Intl.RelativeTimeFormat {}

Honestly, I'm not sure this should be testing all this.  This is basically just `GetPrototypeForConstructor` territory, applied for all constructors.  If that function has targeted testing, that should suffice for here, mostly.  I'd say keep around one test that `new RTF` produces objects with the right prototype, then one test that you *can* subclass RTF, and leave it at that.

::: js/src/tests/Intl/RelativeTimeFormat/format.js:15
(Diff revision 5)
> +addIntlExtras(Intl);
> +
> +
> +rtf = new Intl.RelativeTimeFormat("en-US");
> +assertEq(rtf.format(Date.now()), "now");
> +assertEq(rtf.format(Date.now() - 1000), "1 second ago");

...ooh.  This isn't horribly good.  If execution is slow enough -- maybe the OS pages in another process to run for awhile or something -- this test can fail.  (And so for all the others, with progressively less likelihood.)

It seems to me that RTF needs to have a mode of operation that takes *two* time arguments.  One for the presumed current time, and one for the historical time being talked about.  It seems fine for the default to be to the current moment, but there needs to be a way to override that.  (For a real-world use case, consider a web-based version of Oregon Trail, that displays a journal of things that have happened to you, and wants to say that -- as of some particular time in the game -- eight days prior you tried to ford the river and your oxen died.)

Spec change/spec issue, please come back after you've addressed it there and then in a new patch.  :-)

::: js/src/tests/Intl/RelativeTimeFormat/format.js:30
(Diff revision 5)
> +assertEq(rtf.format(Date.now() + 1000 * 60 * 60 * 24), "tomorrow");
> +
> +assertEq(rtf.format(Date.now() - 1000 * 60 * 60 * 24 * 7), "last week");
> +assertEq(rtf.format(Date.now() + 1000 * 60 * 60 * 24 * 7), "next week");
> +
> +assertEq(rtf.format(Date.now() - 1000 * 60 * 60 * 24 * 32), "last month");

Betcha a dollar this test fails if run *today*.  :-D  Similar failure mode for the test just below as well.

::: js/src/tests/Intl/RelativeTimeFormat/format.js:46
(Diff revision 5)
> +
> +assertEq(rtf.format(Date.now() - 1000 * 60 * 60 * 24 * 7), "1 week ago");
> +assertEq(rtf.format(Date.now() + 1000 * 60 * 60 * 24 * 7), "in 1 week");
> +
> +assertEq(rtf.format(Date.now() - 1000 * 60 * 60 * 24 * 32), "1 month ago");
> +assertEq(rtf.format(Date.now() + 1000 * 60 * 60 * 24 * 32), "in 1 month");

These two are busticated around January 31 and March 1.

::: js/src/tests/Intl/RelativeTimeFormat/format.js:73
(Diff revision 5)
> +  [0,2],
> +  {},
> +];
> +
> +for (let c of weirdCases) {
> +  assertEq(rtf.format(c), "in NaN years");

...okay, so you did consider NaNs and infinities.  I suggest this would be better as just throwing.  *Especially* the NaN case, which is just not something the average web-surfing user is going to have any reason to understand, and we can't expect it of them.

::: js/src/tests/Intl/RelativeTimeFormat/supportedLocalesOf.js:10
(Diff revision 5)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// This array contains the locales that ICU supports in
> +// number formatting whose languages Mozilla localizes Firefox into.
> +// Current as of ICU 50.1.2 and Firefox March 2013.

Remove the "Current as of" bit -- hg revision history should clarify this adequately, and given we're likely to have additions/removals made far down in this list, I expect this would shortly go stale.  Better not to have it.

::: toolkit/components/mozintl/MozIntl.cpp:88
(Diff revision 5)
>  }
>  
> +NS_IMETHODIMP
> +MozIntl::AddRelativeTimeFormatConstructor(JS::Handle<JS::Value> val, JSContext* cx)
> +{
> +  if (!val.isObject()) {

There's a helper function for this now, I think.
Attachment #8826838 - Flags: review?(jwalden+bmo) → review-
Comment on attachment 8826838 [details]
Bug 1270140 - Add mozIntl.RelativeTimeFormat.

https://reviewboard.mozilla.org/r/104710/#review118038

Thanks a lot. I applied some of the feedback already, and will work on the spec updates now, but in the mean time, I have some questions for some of your comments.

> Shouldn't this only be a constructor, now?  That seems the general tack things are taking now.

I don't know how to make it work only as a constructor with the new code from :anba.  It seems like all the optimization functions make it pretty hard to only allow for constructor, no?

> This can fail/return null -- you need to return true/false from this function overall, and the URDTU should be an outparam pointer.

Would it be better if I did the `ensureLinear` in intl_FormatRelativeTime and make `GetBestMatchUnit` infallable?

> The spec shouldn't rely on (an apparently not-specified-in-ECMA-262) round.  For user-visible purposes, rounding 3.5 to 4 but 2.5 to 2 is probably not expected behavior.  Please replace this with something more desirable.

What do you think is more desirable here than `round`?

> There's a helper function for this now, I think.

Umm, can you help me with the name of the function or link to it? I don't see it used anywhere in mozIntl.
Attachment #8826838 - Attachment is obsolete: true
Comment on attachment 8843780 [details]
Bug 1270140 - Add Intl.RelativeTimeFormat.

ugh... not requesting a new review yet!
Attachment #8843780 - Flags: review?(jwalden+bmo)
But am requesting NI on :waldo for comment 16
Flags: needinfo?(jwalden+bmo)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #16)
> > Shouldn't this only be a constructor, now?  That seems the general tack things are taking now.
> 
> I don't know how to make it work only as a constructor with the new code
> from :anba.

    if (!ThrowIfNotConstructing(cx, args, "mozIntl.DateTimeFormat"))
        return false;

or somesuch should do the tree, I think.

> > This can fail/return null -- you need to return true/false from this
> > function overall, and the URDTU should be an outparam pointer.
> 
> Would it be better if I did the `ensureLinear` in intl_FormatRelativeTime
> and make `GetBestMatchUnit` infallable?

That seems fine.  I'm not sure there's strong reason to do it one way or the other.

> > The spec shouldn't rely on (an apparently not-specified-in-ECMA-262)
> > round.  For user-visible purposes, rounding 3.5 to 4 but 2.5 to 2 is
> > probably not expected behavior.  Please replace this with something
> > more desirable.
> 
> What do you think is more desirable here than `round`?

Hmm, looking back at the manpage for round, I was misreading this.  It was more rounding 3.5 to 4 but -3.5 to -4 -- that is, always rounding away from zero.

But setting that aside, I would say that using ToInteger (JS::ToInteger) is the most spec-ready algorithm.

> > There's a helper function for this now, I think.
> 
> Umm, can you help me with the name of the function or link to it? I don't
> see it used anywhere in mozIntl.

Hmm, maybe not.  I was thinking of the AddFunctions helper, but there's no AddConstructor helper.  Probably I should add one.  But go ahead with this for now.
Flags: needinfo?(jwalden+bmo)
We got PluralRules exposed, time to get back to this.

The spec proposal is now at stage 3 and got simplified (no more bestunit!) - https://rawgit.com/tc39/proposal-intl-relative-time/master/index.html

I did not add formatToParts as I believe this will require investment in ICU code, and we don't need to block on this for our internal use.

Welcome back :waldo! Do you have cycle to finish the review? :)
Comment on attachment 8843780 [details]
Bug 1270140 - Add Intl.RelativeTimeFormat.

https://reviewboard.mozilla.org/r/117352/#review192488

Generally looks sane, doing the r- more for volume of nitty things than much in the way of substantive disagreement.  (Although the talk-with-TC39 points might be substantive enough to demand the r- or at least a firm holding-pattern response, maybe.)

::: js/src/builtin/Intl.h:627
(Diff revision 2)
> + * Returns a relative time as a string formatted according to the effective
> + * locale and the formatting options of the given RelativeTimeFormat.
> + *
> + * v should be a number representing a number to be formatted.
> + * u should be a unit string with value being one of:
> + *   second / minute / hour / day / week / month / quarter / year

Probably should just say `u should be "second", "minute", "hour", "day", "week", "month", "quarter", or "year".` as it's just as clear and more concise.

Also, rather than `v`/`u`, could you use `t` and `unit`, or some other clearer mnemonics for the arguments?  (And ideally upstream better names into the spec, if these names do come from the spec.)  I want to say `t` is the typical name for times in ESmumble -- if I'm misremembering, use the actual typical name.

::: js/src/builtin/Intl.cpp:15
(Diff revision 2)
>   */
>  
>  #include "builtin/Intl.h"
>  
>  #include "mozilla/Casting.h"
> +#include "mozilla/MathAlgorithms.h"

This isn't in alphabetical order.

::: js/src/builtin/Intl.cpp:3809
(Diff revision 2)
> +
> +    relativeTimeFormat->setReservedSlot(RelativeTimeFormatObject::INTERNALS_SLOT, NullValue());
> +    relativeTimeFormat->setReservedSlot(RelativeTimeFormatObject::URELATIVE_TIME_FORMAT_SLOT, PrivateValue(nullptr));
> +
> +    RootedValue locales(cx, args.get(0));
> +    RootedValue options(cx, args.get(1));

Make these both

    HandleValue locales = args.get(0);
    HandleValue options = args.get(1);

for a little more efficiency.  Not because efficiency matters here, but because doing it this way here encourages doing it this way elsewhere, and so the idiom has a better chance of propagating into places that really need it (even without authorial intent).

::: js/src/builtin/Intl.cpp:3937
(Diff revision 2)
> +    UDateRelativeDateTimeFormatterStyle relDateTimeStyle = UDAT_STYLE_LONG;
> +    if (StringEqualsAscii(style, "short"))
> +        relDateTimeStyle = UDAT_STYLE_SHORT;
> +    else if (StringEqualsAscii(style, "narrow"))
> +        relDateTimeStyle = UDAT_STYLE_NARROW;
> +

Add

    else {
        MOZ_ASSERT(StringEqualsAscii(style, "long"));
        ...
    }

rather than end with a drop-on-the-floor return-false, please.

::: js/src/builtin/Intl.cpp:3939
(Diff revision 2)
> +        relDateTimeStyle = UDAT_STYLE_SHORT;
> +    else if (StringEqualsAscii(style, "narrow"))
> +        relDateTimeStyle = UDAT_STYLE_NARROW;
> +
> +    JSLinearString* u = args[2].toString()->ensureLinear(cx);
> +    if (!u)

Use a different name than `u` here -- same as in the (modified) comment in that one header, please.

::: js/src/builtin/Intl.cpp:3958
(Diff revision 2)
> +        relDateTimeUnit = UDAT_REL_UNIT_WEEK;
> +    } else if (StringEqualsAscii(u, "month")) {
> +        relDateTimeUnit = UDAT_REL_UNIT_MONTH;
> +    } else if (StringEqualsAscii(u, "quarter")) {
> +        relDateTimeUnit = UDAT_REL_UNIT_QUARTER;
> +    } else if (StringEqualsAscii(u, "year")) {

Looks like this tail should really be

    } else {
        MOZ_ASSERT(StringEqualsAscii(u, "year"));
        relDateTimeUnit = UDAT_REL_UNIT_YEAR;
    }

and there shouldn't be a weird return-false-with-no-error-set, because the only way we should get to this code is if a properly validated unit-string is passed here.

::: js/src/builtin/Intl.cpp:3964
(Diff revision 2)
> +        relDateTimeUnit = UDAT_REL_UNIT_YEAR;
> +    } else {
> +        return false;
> +    }
> +
> +    // ICU doesn't handle -0 well. This worksaround the bug by turning every

"...well: work around this by converting it to +0."

::: js/src/builtin/Intl.cpp:3968
(Diff revision 2)
> +
> +    // ICU doesn't handle -0 well. This worksaround the bug by turning every
> +    // `-0.0` into `0.0`.
> +    // See: http://bugs.icu-project.org/trac/ticket/12936
> +    if (v == 0.0 && signbit(v) == 1) {
> +        v += 0.0;

Instead, `#include "mozilla/FloatingPoint.h"` and `using mozilla::IsNegativeZero;` and

    if (IsNegativeZero(v))
        v = +0.0;

(Technically the + isn't needed, but it seems nicely clearer.)

::: js/src/builtin/Intl.cpp:3974
(Diff revision 2)
> +    }
> +
> +    UErrorCode status = U_ZERO_ERROR;
> +    URelativeDateTimeFormatter* rtf =
> +        ureldatefmt_open(icuLocale(locale.ptr()), nullptr, relDateTimeStyle,
> +                         UDISPCTX_CAPITALIZATION_NONE, &status);

This is a little unfortunate, I guess, having to use `NONE` regardless whether the user wants another context or not.  Do other implementations -- that are open source, at least, and use ICU -- use `NONE` here as well?  I can at least imagine a case for `STANDALONE` being the proper value.  We should sync up with them.

At a higher level: has TC39 considered adding to the API to let the user request a particular context?  Or is this formatting method specified to format with respect to something like a "none" context, such that this is indisputably the correct context to be specifying?

::: js/src/builtin/Intl.js:3491
(Diff revision 2)
> + *
> + * Spec: ECMAScript 402 API, RelativeTimeFormat, 1.3.3.
> + */
> +var relativeTimeFormatInternalProperties = {
> +    localeData: relativeTimeFormatLocaleData,
> +    _availableLocales: null,

If this exactly duplicates the locales for  PluralRules, we could and perhaps should only have one list for them.

But.  If this is a good idea -- depends whether ICU people say the lists could ever diverge, in that one Trac issue mentioned in a comment on another file in this patch -- it certainly needn't/shouldn't be done here and now.  Followup at best.

::: js/src/builtin/Intl.js:3525
(Diff revision 2)
> +    // Step 16.
> +    const r = ResolveLocale(callFunction(RelativeTimeFormat.availableLocales, RelativeTimeFormat),
> +                          lazyRelativeTimeFormatData.requestedLocales,
> +                          lazyRelativeTimeFormatData.opt,
> +                          RelativeTimeFormat.relevantExtensionKeys,
> +                          RelativeTimeFormat.localeData);

Indent all these to line up under the 'c' in 'callFunction'.

::: js/src/builtin/Intl.js:3559
(Diff revision 2)
> +/**
> + * Initializes an object as a RelativeTimeFormat.
> + *
> + * This method is complicated a moderate bit by its implementing initialization
> + * as a *lazy* concept.  Everything that must happen now, does -- but we defer
> + * all the work we can until the object is actually used as a PluralRules.

Change PluralRules.

::: js/src/builtin/Intl.js:3563
(Diff revision 2)
> + * as a *lazy* concept.  Everything that must happen now, does -- but we defer
> + * all the work we can until the object is actually used as a PluralRules.
> + * This later work occurs in |resolveRelativeTimeFormatInternals|; steps not noted
> + * here occur there.
> + *
> + * Spec: ECMAScript 402 API, RelativeTimeFormat, 1.1.1.

I'm guessing 1.1.1 is wrong here -- be sure you're changing this quasi-copypasta?

...or no, looking further down you have 1.blah for all of these.  That's surprising that they'd insert RelativeTimeFormat at the start of the spec, rather than appending to the end of the existing Intl.foo sections -- if they did.  Just double-check and make sure the various 1.foos are correct.

::: js/src/builtin/Intl.js:3567
(Diff revision 2)
> + *
> + * Spec: ECMAScript 402 API, RelativeTimeFormat, 1.1.1.
> + */
> +function InitializeRelativeTimeFormat(relativeTimeFormat, locales, options) {
> +    assert(IsObject(relativeTimeFormat), "InitializeRelativeTimeFormat called with non-object");
> +    assert(IsRelativeTimeFormat(relativeTimeFormat), "InitializeRelativeTimeFormat called with non-RelativeTimeFormat");

Wrap the assertions each to two lines appropriately indented, please, if you can?  I think you can, memory hazy.

::: js/src/builtin/Intl.js:3658
(Diff revision 2)
> +    // Step 4.
> +    let u = ToString(unit);
> +
> +    u = callFunction(std_String_toLowerCase, u);
> +
> +    if (callFunction(ArrayIndexOf, ["second", "minute", "hour", "day", "week", "month", "quarter", "year"], u) === -1) {

I think I'd prefer

    switch (u) {
      case "second":
      case "minute":
      case "hour":
      case "day":
      case "week":
      case "month":
      case "quarter":
      case "year":
        break;
    
      default:
        ThrowRangeError(...);
    }

::: js/src/tests/Intl/RelativeTimeFormat/format.js:14
(Diff revision 2)
> +
> +addIntlExtras(Intl);
> +
> +
> +rtf = new Intl.RelativeTimeFormat("en-US");
> +assertEq(rtf.format(0, "second"), "now");

-0 as well?  And add -0 testing anywhere else you test 0 (and add 0 tests for all the rest of these units, please).

::: js/src/tests/Intl/RelativeTimeFormat/supportedLocalesOf.js:10
(Diff revision 2)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// This array contains the locales that ICU supports in
> +// number formatting whose languages Mozilla localizes Firefox into.
> +// Current as of ICU 50.1.2 and Firefox March 2013.

This currency comment seems dubiously accurate, but if it really is accurate, okay.  :-)
Attachment #8843780 - Flags: review?(jwalden+bmo) → review-
Comment on attachment 8843780 [details]
Bug 1270140 - Add Intl.RelativeTimeFormat.

https://reviewboard.mozilla.org/r/117352/#review192522

I should note that I didn't compare/contrast this patch with the actual spec algorithms, just looked for sanity/expectations presuming the algorithm was accurate.  This seems fairly straightforward so there probably isn't much room for mistake on that front, but it's something I'll have to look at in the next pass or so.

::: js/src/builtin/Intl.cpp:3937
(Diff revision 2)
> +    UDateRelativeDateTimeFormatterStyle relDateTimeStyle = UDAT_STYLE_LONG;
> +    if (StringEqualsAscii(style, "short"))
> +        relDateTimeStyle = UDAT_STYLE_SHORT;
> +    else if (StringEqualsAscii(style, "narrow"))
> +        relDateTimeStyle = UDAT_STYLE_NARROW;
> +

...and put the other arms in the normal braces at the same time, of course.
Comment on attachment 8843780 [details]
Bug 1270140 - Add Intl.RelativeTimeFormat.

https://reviewboard.mozilla.org/r/117352/#review192488

> This is a little unfortunate, I guess, having to use `NONE` regardless whether the user wants another context or not.  Do other implementations -- that are open source, at least, and use ICU -- use `NONE` here as well?  I can at least imagine a case for `STANDALONE` being the proper value.  We should sync up with them.
> 
> At a higher level: has TC39 considered adding to the API to let the user request a particular context?  Or is this formatting method specified to format with respect to something like a "none" context, such that this is indisputably the correct context to be specifying?

We're the first to implement this API. I switched to STANDALONE, because I think it makes sense and I'll try to consult ECMA402 on the plans here.

> If this exactly duplicates the locales for  PluralRules, we could and perhaps should only have one list for them.
> 
> But.  If this is a good idea -- depends whether ICU people say the lists could ever diverge, in that one Trac issue mentioned in a comment on another file in this patch -- it certainly needn't/shouldn't be done here and now.  Followup at best.

I'll verify it with Dan, but I believe that we'd like to push on ICU to get to expose available locales per API (so PluralRules and RelativeTimeFormat may end up with different sets).

> I'm guessing 1.1.1 is wrong here -- be sure you're changing this quasi-copypasta?
> 
> ...or no, looking further down you have 1.blah for all of these.  That's surprising that they'd insert RelativeTimeFormat at the start of the spec, rather than appending to the end of the existing Intl.foo sections -- if they did.  Just double-check and make sure the various 1.foos are correct.

it is correct. I'm following the spec proposal which is here: https://rawgit.com/tc39/proposal-intl-relative-time/master/index.html
That's what we did with PluralRules, and I'll update both when they get merged into the full spec.

Numbering is a subject to a never-ending chase, since the spec will change numbers all around (including algo steps) between versions, so my take is that I try to keep the numbers relevant to the spec/proposal revision and update them when I update the related code.

> This currency comment seems dubiously accurate, but if it really is accurate, okay.  :-)

I copied the list of locales from a test that had it. Since I didn't touch the locale list, I kept the comment.
Oh, and:

> At a higher level: has TC39 considered adding to the API to let the user request a particular context?  Or is this formatting method specified to format with respect to something like a "none" context, such that this is indisputably the correct context to be specifying?

Oh, and yes, this has been raised as a potential enhancement for the next revision of this API (post 1.0) - https://github.com/tc39/proposal-intl-relative-time/issues/11
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #25)
> I'll verify it with Dan, but I believe that we'd like to push on ICU to get
> to expose available locales per API (so PluralRules and RelativeTimeFormat
> may end up with different sets).

Yeah, that's what I'd expect.  I wonder if we could share "similar" list representation somehow, not just exact replication.  Still for another bug.

> it is correct. I'm following the spec proposal which is here:
> https://rawgit.com/tc39/proposal-intl-relative-time/master/index.html
> That's what we did with PluralRules, and I'll update both when they get
> merged into the full spec.

Ah, makes sense.
Comment on attachment 8843780 [details]
Bug 1270140 - Add Intl.RelativeTimeFormat.

https://reviewboard.mozilla.org/r/117352/#review192838

::: js/src/builtin/Intl.cpp:16
(Diff revisions 2 - 3)
>  #include "builtin/Intl.h"
>  
>  #include "mozilla/Casting.h"
> -#include "mozilla/MathAlgorithms.h"
>  #include "mozilla/HashFunctions.h"
> +#include "mozilla/FloatingPoint.h"

F before H.

::: js/src/builtin/Intl.cpp:776
(Diff revisions 2 - 3)
> +URelativeDateTimeFormatter*
> +ureldatefmt_open( const char*          locale,
> +                  UNumberFormat*       nfToAdopt,
> +                  UDateRelativeDateTimeFormatterStyle width,
> +                  UDisplayContext      capitalizationContext,
> +                  UErrorCode*          status )

Formatting should omit the vertical column of space after the ')' and before the ')'.  Also, please don't align the parameter names -- vertical alignment internal to lines (rather than at the start) is mostly a nuisance, and IMO not even more readable.  Also fix this below, too.

::: js/src/builtin/Intl.cpp:3983
(Diff revisions 2 - 3)
> -    if (StringEqualsAscii(style, "short"))
> +    if (StringEqualsAscii(style, "short")) {
>          relDateTimeStyle = UDAT_STYLE_SHORT;
> -    else if (StringEqualsAscii(style, "narrow"))
> +    } else if (StringEqualsAscii(style, "narrow")) {
>          relDateTimeStyle = UDAT_STYLE_NARROW;
> +    } else {
> +        MOZ_ASSERT(StringEqualsAscii(style, "long"));

Move the setting of `relDateTimeStyle` into this block, please, consistent with how `relDateTimeUnit` is defined/set.

::: js/src/builtin/Intl.js:3658
(Diff revisions 2 - 3)
> -    let v = ToNumber(value);
> +    let t = ToNumber(value);
>  
>      // Step 4.
>      let u = ToString(unit);
>  
>      u = callFunction(std_String_toLowerCase, u);

I don't see a lowercasing operation in the spec -- it just accepts the exact literal strings "second" and so on.  If any of these tests are likely to be upstreamed at all, add a test verifying that a few strings like "sEcond" and so on throw a RangeError?

::: js/src/tests/Intl/RelativeTimeFormat/format.js:25
(Diff revisions 2 - 3)
> +assertEq(rtf.format(-0, "minute"), "in 0 minutes");
>  assertEq(rtf.format(-1, "minute"), "1 minute ago");
>  assertEq(rtf.format(1, "minute"), "in 1 minute");
>  
> +assertEq(rtf.format(0, "hour"), "in 0 hours");
> +assertEq(rtf.format(-0, "hour"), "in 0 hours");

This seems like a peculiar behavior for ICU to have -- "now" seems best for seconds/minutes (maybe hours too), not just for seconds -- but eh, whatevs.
Attachment #8843780 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8843780 [details]
Bug 1270140 - Add Intl.RelativeTimeFormat.

https://reviewboard.mozilla.org/r/117352/#review192838

> I don't see a lowercasing operation in the spec -- it just accepts the exact literal strings "second" and so on.  If any of these tests are likely to be upstreamed at all, add a test verifying that a few strings like "sEcond" and so on throw a RangeError?

I removed this and added a test to throw on bad casing.

> This seems like a peculiar behavior for ICU to have -- "now" seems best for seconds/minutes (maybe hours too), not just for seconds -- but eh, whatevs.

My guess is that for higher level libraries that will use it, they can coalesce all (0, unit) for time into (0, "second") and for date into (0, "day").
https://hg.mozilla.org/mozilla-central/rev/9189c75c416f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Blocks: 1504334
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: