1287503 - Expose an API to get the calendar information

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect)

Description

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

As part of the effort in bug 1280654 to get a set of APIs for date/time UIs, we need a simple API function to get the first day of the week.

Based on our experience with Firefox OS [0] and proposals for ECMA402 [1], I suggest:

Intl.getFirstDayOfTheWeek(locale);

The function will return an integer for a day on which the week starts. Initially, there will be no options, so we'll use gregorian calendar, but in the future we may be able to set the calendar. If locale argument is omitted, it'll take the system locale.

0 - Sunday
1 - Monday
2 - Tuesday
etc.


[0] https://github.com/mozilla-b2g/gaia/blob/master/shared/js/moz_intl.js
[1] https://github.com/tc39/ecma402/issues/6

Comment 1

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

two updates:

1) The name of the function is getFirstDayOfWeek (no "The")
2) 1 is Sunday, 2 is Monday and so on. to stay in line with CLDR.

Comment 2

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

Attachment: bug1287503.diff

The initial patch.

It's pretty small, so should be easier to review! :)

What I'd like to know is where would you like me to locate it (I remember you saying that you'd prefer to keep the chrome-only stuff on different object than Intl), and how to guard it for chrome-only.

Comment 3

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

Comment on attachment 8772107 [details] [diff] [review]
bug1287503.diff

Actually, screw that.

Scott needs weekendStarts/weekendEnds as well, and this API doesn't return the resolved locale info.

Comment 4

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

New proposal based on https://github.com/tc39/ecma402/issues/46

mozIntl.getCalendarInfo(locales);

returns:

{
  'firstDayOfWeek': 1,
  'weekendStarts': 6,
  'weekendEnds': 7,
  'locale': 'de',
}

This may be extended in the future to return:

 - calendar that is used by the selected locale
 - minimum amount of days in a week
 - etc.

and if we'll ever decide to add 'options' as a second parameter, it'll be compatible with other Intl APIs.

Comment 5

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

Attachment: bug1287503.diff

This is better.

It adds Intl.getCalendarInfo(locales) which returns an object:

{
  "firstDayOfWeek": 7,
  "weekendStart": 6,
  "weekendEnd": 1,
  "locale": "de"
  "calendar": "gregory"
}

Comment 6

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

Waldo: questions for you:

 - where should it live? (I remember you mentioning that maybe not on Intl)
 - how to guard it for chrome code only
 - any other feedback/concerns before I write more tests and submit a formal review request :)

Comment 7

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

Scott, can you confirm that an API as described in comment 5 will work for you?

Comment 8

[Scott Wu [:scottwu]]

Yes I think this will work just fine. I'll just need to map the CLDR format to JS.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5)
> {
>   "firstDayOfWeek": 7,
>   "weekendStart": 6,
>   "weekendEnd": 1,
>   "locale": "de"
>   "calendar": "gregory"
> }

In this case the first day of the week is Saturday? and the weekend is Friday to Sunday?

Comment 9

[Scott Wu [:scottwu]]

Actually, I see what you mean now looking at your test cases.

> assertEq(deepEqual(gCI('en-US'), {
>   "firstDayOfWeek": 1,
>   "weekendStart": 7,
>   "weekendEnd": 2,
>   "calendar": "gregory",
>   "locale": "en-US"
> }), true);

The last day of the weekend is actually (weekendEnd - 1), yes?

Comment 10

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

Sorry, that was of course just a random example.

Real ones would be:

for 'en-US':
{
  "firstDayOfWeek": 1,
  "weekendStart": 7,
  "weekendEnd": 2,
  "calendar": "gregory",
  "locale": "en-US"
}

for 'en-GB':

{
  "firstDayOfWeek": 2,
  "weekendStart": 7,
  "weekendEnd": 2,
  "calendar": "gregory",
  "locale": "en-GB"
}

where 1: Sunday, 2: Monday, 3: Tuesday etc.

Comment 11

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

:srl - does it look sane to you from the ICU perspective?

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 8772181 [details] [diff] [review]
bug1287503.diff

Review of attachment 8772181 [details] [diff] [review]:
-----------------------------------------------------------------

So you're going to hate this, but that's how locale-sensitivity works.  Everything's more difficult than you want it to be.

Should we be assuming a week is seven days everywhere?  Even if so, should we be assuming a week is the unit of granularity people want for this?  And because this is sort of for displaying calendars, I guess, should it be conditioned on a particular instant in time?  Various places have defined "week" as not a seven-day period at various times.  France had a ten-day week from 1793-1802.  The USSR had five-day and six-day weeks from 1929-1940.  https://en.wikipedia.org/wiki/Week#.22Weeks.22_in_other_calendars describes other deviations.  Should a new API provide information for those systems?  Seems like it possibly should -- and if so, the bare information provided by this API seems like it would be inadequate to describe those other systems.  I suspect you'd need something more elaborate (e.g. including an array of strings naming days of the week or so), with different API surface.  I'm not necessarily convinced it's a good idea to standardize only on a modern concept of a week (supposing seven-day weeks happen everywhere) and ignore historical considerations, given that people will want to have calendars and the like that correspond to those times.

But having raised the issue, I'm basically happy to defer to an i18n-considerate person like Steven for actual answers here.  This is his domain of expertise, not mine -- I can just attempt to poke holes in ideas, and implement them somewhat.  :-)

::: js/src/builtin/Intl.cpp
@@ +2337,5 @@
> +
> +    JSAutoByteString locale(cx, args[0].toString());
> +
> +    UErrorCode status = U_ZERO_ERROR;
> +    UCalendar* cal = ucal_open(nullptr, 0, locale.ptr(), UCAL_DEFAULT, &status);

This needs an error-check.  Then once it's succeeded, some sort of RAII object is needed to close the calendar when the function returns.  Right now a failure to allocate an object, or define one of the properties on it, will leak this UCalendar*.

@@ +2340,5 @@
> +    UErrorCode status = U_ZERO_ERROR;
> +    UCalendar* cal = ucal_open(nullptr, 0, locale.ptr(), UCAL_DEFAULT, &status);
> +    int32_t firstDayOfWeek = ucal_getAttribute(cal, UCAL_FIRST_DAY_OF_WEEK);
> +
> +    RootedObject info(cx, JS_NewPlainObject(cx));

Inside the JS engine we'll want an internal API for this.  I suspect Intl.cpp has something that demonstrates how to do this -- NewBuiltinClassInstance<PlainObject>(cx) maybe, as formatToParts does.

@@ +2344,5 @@
> +    RootedObject info(cx, JS_NewPlainObject(cx));
> +    if (!info)
> +        return false;
> +
> +    RootedValue firstDayOfWeekVal(cx, NumberValue(firstDayOfWeek));

You can use Int32Value if you have a definite int32_t to work with, which you do.

@@ +2349,5 @@
> +    RootedValue weekendStartVal(cx);
> +    RootedValue weekendEndVal(cx);
> +
> +    if (!JS_DefineProperty(cx, info, "firstDayOfWeek", firstDayOfWeekVal, JSPROP_ENUMERATE))
> +        return false;

Our internal DefineProperty API is preferable to using JS_DefineProperty for this stuff.  The signatures should mostly carry over, so it shouldn't be too much trouble changing.

@@ +2351,5 @@
> +
> +    if (!JS_DefineProperty(cx, info, "firstDayOfWeek", firstDayOfWeekVal, JSPROP_ENUMERATE))
> +        return false;
> +
> +    UCalendarWeekdayType prevDayType = ucal_getDayOfWeekType(cal, UCAL_SATURDAY, &status);

Error-check.

@@ +2355,5 @@
> +    UCalendarWeekdayType prevDayType = ucal_getDayOfWeekType(cal, UCAL_SATURDAY, &status);
> +
> +    for (int i = UCAL_SUNDAY; i <= UCAL_SATURDAY; i++) {
> +        UCalendarDaysOfWeek dayOfWeek = (UCalendarDaysOfWeek) i;
> +        UCalendarWeekdayType type = ucal_getDayOfWeekType(cal, dayOfWeek, &status);

Error check.

@@ +2366,5 @@
> +                case UCAL_WEEKEND:
> +                    weekendStartVal = NumberValue(i);
> +                    break;
> +                default:
> +                    MOZ_ASSERT_UNREACHABLE("unhandled dayOfWeek type");

http://icu-project.org/apiref/icu4c/ucal_8h.html#a52650ccf4dae0f42dd82624a94087c61 clearly documents that this *is* reachable.  Whatever spec algorithm is proposed for this is going to have to contemplate this edge case.  (Again, sorry -- that's how locale-sensitive info works.  Everything you want to assume, you can't assume, usually.)

@@ +2380,5 @@
> +    if (!JS_DefineProperty(cx, info, "weekendEnd", weekendEndVal, JSPROP_ENUMERATE))
> +        return false;
> +
> +    args.rval().setObject(*info);
> +    ucal_close(cal);

As I said, earlier failures mean you never reach this and leak.  *Any* time you have resource-freeing on final non-error return, you almost certainly need RAII to not leak in error cases.

::: js/src/builtin/Intl.h
@@ +187,5 @@
> + * Returns: {
> + *   firstDayOfWeek: 1,
> + *   weekendStart: 6,
> + *   weekendEnd: 1
> + * }

If this were to stay as set currently, these numbers would *definitely* require documentation as to what they correspond to, especially with the 0/1 distinction you want to make.  (I'm not sure either way whether Sunday should be 1, but it's totally non-obvious which would be which, for sure.)

Comment 13

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

Hi Jeff,

thanks for the feedback round1! :)

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #12)
> So you're going to hate this, but that's how locale-sensitivity works. 
> Everything's more difficult than you want it to be.

Hahaha, I think I know what you mean ;)
 
> Should we be assuming a week is seven days everywhere?  Even if so, should
> we be assuming a week is the unit of granularity people want for this?

Yes.

That's a known limitation of the API at this point. And that's OK I believe at this stage of development. We're exposing an internal API that will be used by our date/time formats (that currently only handle gregorian calendar).

In the future, once we start taking lessons from this API as we work on designing ECMA402 API, we will make it either multi-calendar, or forward-compatible to allow us to add other calendar systems.

> I'm not necessarily convinced it's a good idea to standardize only on a modern
> concept of a week (supposing seven-day weeks happen everywhere) and ignore
> historical considerations, given that people will want to have calendars and
> the like that correspond to those times.

We're not standardizing this API, we're exposing it so that we can use it in Firefox (and seems like Lightning is interested), and use the lesson once we get to more work around https://github.com/tc39/ecma402/issues/46

Comment 14

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

Attachment: patch v2

Updated version of the patch.

This would be a good candidate to be the first to land in mozIntl, but I'm not sure how to solve the chicken-and-egg problem here. I don't want to expose it on Intl object, but I'm not sure which piece we'll be referenced from the toolkit mozIntl - the JS function or the CPP function?

Also, I still need a bit of help to get the test to launch if we don't expose it on the Intl object.

The rest should be ready for review.

Comment 15

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

I resolved the chicken-and-egg problem in my head.

The patch here will not expose it anywhere except for tests.

The patch in bug 1289951 will enable usage of this feature.

The only thing remaining is how to expose this to tests. Do I need to duplicate the "AddGetCalendarInfo" from mozIntl?

Comment 16

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

Attachment: patch v3

Added addIntlExtras to JS Shell and removed the exposure on the Intl object.

I think it's ready for your review Waldo :)

Comment 17

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

Attachment: patch v4

Updated with feedback applied from bz from bug 1289951

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 8775750 [details] [diff] [review]
patch v4

Review of attachment 8775750 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Intl.cpp
@@ +2361,5 @@
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    MOZ_ASSERT(args.length() == 1);
> +
> +    JSAutoByteString locale(cx, args[0].toString());

This requires

    if (!locale)
        return false;

to handle out-of-memory.

@@ +2377,5 @@
> +    RootedObject info(cx, NewBuiltinClassInstance<PlainObject>(cx));
> +    if (!info)
> +        return false;
> +
> +    RootedValue firstDayOfWeekVal(cx, Int32Value(firstDayOfWeek));

Prefer declaring/assigning variables as close to use as possible.  So then,

    int32_t firstDayOfWeek = ucal_getAttribute(cal, UCAL_FIRST_DAY_OF_WEEK);
    RootedValue firstDayOfWeekVal(cx, Int32Value(firstDayOfWeek));
    if (!DefineProperty(...))
        return false;

And then only just before the loop below, should you have

    RootedValue weekendStart(cx), weekendEnd(cx);

(with "val" taken off the names -- that's usually when there are two redundant values, but there aren't here).

@@ +2391,5 @@
> +        return false;
> +    }
> +
> +    for (int i = UCAL_SUNDAY; i <= UCAL_SATURDAY; i++) {
> +        UCalendarDaysOfWeek dayOfWeek = (UCalendarDaysOfWeek) i;

static_cast<UCalendarDaysOfWeek>(i) to use a C++-style cast.

@@ +2401,5 @@
> +
> +        if (prevDayType != type) {
> +            switch (type) {
> +                case UCAL_WEEKDAY:
> +                    weekendEndVal = NumberValue(i);

switch (type) {
  case UCAL_WEEKDAY:
    weekendEnd.setInt32(i);
    break;
  case UCAL_WEEKEND:
    weekendStart.setInt32(i);
    break;
}

for the formatting -- cases/defaults indented two spaces, their subsequent "bodies" a further two spaces indented.

It's clearly not right to have a default here that asserts unreachable.  At an absolute minimum, add a default section that just breaks (break on a separate line, of course) to ignore that.

@@ +2404,5 @@
> +                case UCAL_WEEKDAY:
> +                    weekendEndVal = NumberValue(i);
> +                    break;
> +                case UCAL_WEEKEND:
> +                    weekendStartVal = NumberValue(i);

But, actually.  In the course of writing the description of this function for Intl.h, I applied the patch and did some tests, and I get this behavior:

js> Intl.getCalendarInfo("en-US")
({firstDayOfWeek:1, weekendStart:7, weekendEnd:2, calendar:"gregory", locale:"en-US"})

I would have expected weekendEnd to be 1 here -- that is, weekendEnd I would expect to be inclusive, not exclusive.

js> Intl.getCalendarInfo("en-IL")
({firstDayOfWeek:1, weekendStart:6, weekendEnd:1, calendar:"gregory", locale:"en-IL"})

And here, I'd expect weekendEnd to be 7.

js> Intl.getCalendarInfo("fa-IR")
({firstDayOfWeek:7, weekendStart:6, weekendEnd:7, calendar:"persian", locale:"fa-IR"})

And in Iran I would expect weekendStart and weekendEnd to both be 6, because of its single-day weekend.  http://financialtribune.com/articles/economy-domestic-economy/42680/calls-changing-iran%E2%80%99s-weekend-structure

Why does the implementation of the API, as you've done it here, make the end date as exclusive?  Why isn't it inclusive?

@@ +2407,5 @@
> +                case UCAL_WEEKEND:
> +                    weekendStartVal = NumberValue(i);
> +                    break;
> +                default:
> +                    MOZ_ASSERT_UNREACHABLE("unhandled dayOfWeek type");

As far as I can tell, ICU doesn't *actually* return UCAL_WEEKEND_ONSET or UCAL_WEEKEND_CEASE, because no region has weekends that start or end midday.  But 1) I might have misread the relevant data files, 2) I might have misread the code, and 3) ICU could easily add this possibility in the future, and we might not realize it when we update.

So we need to somehow handle the other two cases.  We have decent compiler warning coverage, and we pounce on compiler warnings pretty quickly, so it should be fine to exhaustively enumerate and handle the cases.  As to what those cases should do, I think we want to know immediately if this happens, so we should make it throw an exception:

  case UCAL_WEEKEND_ONSET:
  case UCAL_WEEKEND_CEASE:
    // At the time this code was added, ICU apparently never behaves this way,
    // so just throw, so that users will report a bug and we can decide what to
    // do.
    JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_INTERNAL_INTL_ERROR);
    return false;

@@ +2413,5 @@
> +        }
> +
> +        prevDayType = type;
> +    }
> +

Add

  MOZ_ASSERT(weekendStart.isInt32());
  MOZ_ASSERT(weekendEnd.isInt32());

to verify that we've properly found both values here -- separated from the } by a blank line, separated from the first DefineProperty by a blank line.

@@ +2421,5 @@
> +    if (!DefineProperty(cx, info, cx->names().weekendEnd, weekendEndVal))
> +        return false;
> +
> +    args.rval().setObject(*info);
> +    return toClose.forget();

Uh...

This should just be |return true;|.  |toClose| will do the closing when it falls out of scope on its own.  And |toClose.forget();| would actually *leak* the calendar and never ever close it, so it's doubly odd doing it this way.

::: js/src/builtin/Intl.h
@@ +183,4 @@
>  
>  #if ENABLE_INTL_API
>  /**
> + * Returns an Object with calendar infromation for a given locale.

A better/clearer/more detailed doc comment for this (but, updated for inclusive weekendDay, as my Intl.cpp comments discuss):

/**
 * Returns a plain object with calendar information for a single valid locale
 * (callers must perform this validation).  The object will have these
 * properties:
 *
 *   firstDayOfWeek
 *     an integer in the range 1=Sunday to 7=Saturday indicating the day
 *     considered the first day of the week in calendars, e.g. 1 for en-US,
 *     2 for en-GB, 1 for in-IN
 *   weekendStart
 *     an integer in the range 1=Sunday to 7=Saturday indicating the day
 *     considered the beginning of a weekend, e.g. 7 for en-US, 7 for en-GB,
 *     1 for in-IN
 *   weekendEnd
 *     an integer in the range 1=Sunday to 7=Saturday indicating the day
 *     considered the end of a weekend, e.g. 1 for en-US, 1 for en-GB,
 *     1 for in-IN (note that "weekend" is *not* necessarily two days)
 *
 * NOTE: "calendar" and "locale" properties are *not* added to the object.
 */

::: js/src/builtin/Intl.js
@@ +2895,5 @@
> +  const r = ResolveLocale(callFunction(DateTimeFormat.availableLocales, DateTimeFormat),
> +    requestedLocales,
> +    localeOpt,
> +    DateTimeFormat.relevantExtensionKeys,
> +    localeData);

Align all these arguments with the "c" in callFunction.

::: js/src/shell/js.cpp
@@ +729,5 @@
> +AddIntlExtras(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    if (!args.get(0).isObject())
> +        return false;

In JSAPI terms, you should only ever return false when you've reported an error, thrown an exception, etc.  That hasn't happened here -- you've just observed a value to not be what you wanted.  This should be:

    CallArgs args = CallArgsFromVp(argc, vp);

    if (!args.get(0).isObject()) {
        JS_ReportError(cx, "addIntlExtras must be passed an object");
        return false;
    }

so that failing to pass an object throws an exception.  As it is, the failure would *silently stop all JS execution*, which is not what you want.  (This behavior is what we do for OOM handling in many cases, but for run-of-the-mill misuses it's totally wrong.)

@@ +731,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    if (!args.get(0).isObject())
> +        return false;
> +    JS::RootedObject intl(cx, &args.get(0).toObject());
> +    if (!intl)

This operation isn't fallible, once it's known the first argument is an object.  So no need for a null-check.

@@ +5724,5 @@
>  "Sets the callback to be invoked whenever a Promise rejection is unhandled\n"
>  "or a previously-unhandled rejection becomes handled."),
>  
> +    JS_FN_HELP("addIntlExtras", AddIntlExtras, 1, 0,
> +"AddIntlExtras(obj)",

"addIntlExtras(obj)"

@@ +5726,5 @@
>  
> +    JS_FN_HELP("addIntlExtras", AddIntlExtras, 1, 0,
> +"AddIntlExtras(obj)",
> +"Adds a set of extra Intl functions that are not yet standardized on Intl object\n"
> +" and allows the user to add them on an object passed as an argument."),

"Adds various not-yet-standardized Intl functions as properties on the\n"
"provided object (this should generally be Intl itself).  The added\n"
"functions and their behavior are experimental: don't depend upon them\n"
"unless you're willing to update your code if these experimental APIs change\n"
"underneath you."),

::: js/src/tests/Intl/getCalendarInfo.js
@@ +10,5 @@
> + * means that they have the same set of enumerable properties, the values
> + * of each property are deep_equal, and their 'length' properties are
> + * equal. Equality on other types is ==.
> + */
> +function deepEqual(a, b) {

This is overkill.  Write a custom function that compares enumerated values:

function checkCalendarInfo(info, expected)
{
  assertEq(Object.getPrototypeOf(info), Object.prototype);

  assertEq(info.firstDayOfWeek, expected.firstDayOfWeek);
  assertEq(info.weekendStart, expected.weekendStart);
  assertEq(info.weekendEnd, expected.weekendEnd);
  assertEq(info.calendar, expected.calendar);
  assertEq(info.locale, expected.locale);
}

@@ +45,5 @@
> +
> +assertEq(gCI.length, 1);
> +
> +assertEq(deepEqual(gCI('en-US'), {
> +  "firstDayOfWeek": 1,

Remove the quotes around the property names -- they're just visual noise here, IMO.

Comment 19

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

Attachment: patch v5

Applied your feedback - including updating the algo for weekendEnd.

I also added one more variable - 'minDays' following https://github.com/unicode-cldr/cldr-core/blob/master/supplemental/weekData.json

This variable is commonly used by calendar UIs to layout month view for January.

Comment 20

[Jeff Walden [:Waldo]]

Comment on attachment 8780415 [details] [diff] [review]
patch v5

Review of attachment 8780415 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Intl.cpp
@@ +2345,5 @@
> +    if (!locale)
> +        return false;
> +
> +    UErrorCode status = U_ZERO_ERROR;
> +    UCalendar* cal = ucal_open(nullptr, 0, locale.ptr(), UCAL_DEFAULT, &status);

Rather than nullptr/0 here, have

  const UChar* uTimeZone = nullptr;
  int32_t uTimeZoneLength = 0;

just above the |status| declaration, then pass those instead of the literals.  This is a nicety other Intl.cpp code has adopted for similar parameters -- it's good documentation, especially for when we start doing things with time zones.

@@ +2352,5 @@
> +        return false;
> +    }
> +    ScopedICUObject<UCalendar, ucal_close> toClose(cal);
> +
> +

Remove one of these blank lines.

@@ +2357,5 @@
> +    RootedObject info(cx, NewBuiltinClassInstance<PlainObject>(cx));
> +    if (!info)
> +        return false;
> +
> +    int32_t firstDayOfWeek = ucal_getAttribute(cal, UCAL_FIRST_DAY_OF_WEEK);

MOZ_ASSERT(1 <= firstDayOfWeek && firstDayOfWeek <= 7,
           "bad first day of week?");

@@ +2358,5 @@
> +    if (!info)
> +        return false;
> +
> +    int32_t firstDayOfWeek = ucal_getAttribute(cal, UCAL_FIRST_DAY_OF_WEEK);
> +    RootedValue firstDayOfWeekVal(cx, Int32Value(firstDayOfWeek));

Above this firstDayOfWeek definition, separated by a blank line, add

    RootedValue v(cx);

and then do |v.setInt32(firstDayOfWeek)| and |v.setInt32(minDays)|.  We don't need multiple Rooteds for all the separate things, and there's some cost associated with not reusing Rooteds when possible.  And for one-off, throwaway uses like this it's entirely clear if we reuse.

@@ +2362,5 @@
> +    RootedValue firstDayOfWeekVal(cx, Int32Value(firstDayOfWeek));
> +    if (!DefineProperty(cx, info, cx->names().firstDayOfWeek, firstDayOfWeekVal))
> +        return false;
> +
> +    int32_t minDays = ucal_getAttribute(cal, UCAL_MINIMAL_DAYS_IN_FIRST_WEEK);

MOZ_ASSERT(1 <= minDays && minDays <= 7, "bad minimal days in first week");

::: js/src/builtin/Intl.h
@@ +189,5 @@
> + *
> + *   firstDayOfWeek
> + *     an integer in the range 1=Sunday to 7=Saturday indicating the day
> + *     considered the first day of the week in calendars, e.g. 1 for en-US,
> + *     2 for en-GB, 1 for in-IN

Pretty sure I messed up in-IN (Indonesian as used in India?) -- use bn-IN (Bengali as used in India) instead.  The India part's the important part, but this should refer to a language actually used in India.

@@ +195,5 @@
> + *     an integer in the range of 1 to 7 indicating the minimum number
> + *     of days required in the first week of the year
> + *     an integer in the range 1=Sunday to 7=Saturday indicating the day
> + *     considered the first day of the week in calendars,
> + *     e.g. 1 for en-US, 4 for de

Are lines 197-199 copypasta?

@@ +199,5 @@
> + *     e.g. 1 for en-US, 4 for de
> + *   weekendStart
> + *     an integer in the range 1=Sunday to 7=Saturday indicating the day
> + *     considered the beginning of a weekend, e.g. 7 for en-US, 7 for en-GB,
> + *     1 for in-IN

bn-IN

@@ +203,5 @@
> + *     1 for in-IN
> + *   weekendEnd
> + *     an integer in the range 1=Sunday to 7=Saturday indicating the day
> + *     considered the end of a weekend, e.g. 1 for en-US, 1 for en-GB,
> + *     1 for in-IN (note that "weekend" is *not* necessarily two days)

bn-IN

::: js/src/shell/js.cpp
@@ +733,5 @@
> +    if (!args.get(0).isObject()) {
> +        JS_ReportError(cx, "addIntlExtras must be passed an object");
> +        return false;
> +    }
> +    JS::RootedObject intl(cx, &args.get(0).toObject());

Use &args[0].toObject() here -- .get() length-checks and substitutes |undefined| if needed, but we already verified we have an object, so better to use the non-length-checking one here.

::: js/src/tests/Intl/getCalendarInfo.js
@@ +60,5 @@
> +  calendar: "gregory",
> +  locale: "pl"
> +});
> +
> +checkCalendarInfo(gCI('ar'), {

This seems like a bad test to have.  This is Arabic generically, but different Arabic locales have different values for this stuff: ar-IL versus ar-IQ, for example.  Either qualify this with a particular region, or omit it.  (Polish is different in that it's associated with one region, so pl implies pl-PL.)

::: js/src/vm/CommonPropertyNames.h
@@ +110,4 @@
>      macro(Float32x4, Float32x4, "Float32x4") \
>      macro(float64, float64, "float64") \
>      macro(Float64x2, Float64x2, "Float64x2") \
> +    macro(firstDayOfWeek, firstDayOfWeek, "firstDayOfWeek") \

fi < fl, so this should go upward somewhere.

@@ +309,5 @@
>      macro(RegExpSearcher, RegExpSearcher, "RegExpSearcher") \
>      macro(RegExpTester, RegExpTester, "RegExpTester") \
>      macro(weekday, weekday, "weekday") \
> +    macro(weekendStart, weekendStart, "weekendStart") \
> +    macro(weekendEnd, weekendEnd, "weekendEnd") \

Flip these two to be (at least locally) alphabetical.

Comment 21

[Steven R. Loomis]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #20)
> Comment on attachment 8780415 [details] [diff] [review]
> patch v5
> 


> @@ +2362,5 @@
> > +    RootedValue firstDayOfWeekVal(cx, Int32Value(firstDayOfWeek));
> > +    if (!DefineProperty(cx, info, cx->names().firstDayOfWeek, firstDayOfWeekVal))
> > +        return false;
> > +
> > +    int32_t minDays = ucal_getAttribute(cal, UCAL_MINIMAL_DAYS_IN_FIRST_WEEK);
> 
> MOZ_ASSERT(1 <= minDays && minDays <= 7, "bad minimal days in first week");
> 

Why ? You don't actually know that's the range… Could call http://icu-project.org/apiref/icu4c/ucal_8h.html#a5f43bd7f846d9dbc94a9a46af26e7ab7 ucal_getLimit(cal, UCAL_DAY_OF_WEEK, UCAL_MAXIMUM, status) if you really wanted to

Comment 22

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/81723d4f90ed804b7a29d70aa1408b1f81a335e9
Bug 1287503 - Expose a JS Intl API to get calendar information. r=waldo

Comment 23

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

Lande it without the assertion in question.

Comment 24

[Jeff Walden [:Waldo]]

(In reply to Steven R. Loomis from comment #21)
> > > +    int32_t minDays = ucal_getAttribute(cal, UCAL_MINIMAL_DAYS_IN_FIRST_WEEK);
> > 
> > MOZ_ASSERT(1 <= minDays && minDays <= 7, "bad minimal days in first week");
> > 
> 
> Why ? You don't actually know that's the range… Could call
> http://icu-project.org/apiref/icu4c/ucal_8h.
> html#a5f43bd7f846d9dbc94a9a46af26e7ab7 ucal_getLimit(cal, UCAL_DAY_OF_WEEK,
> UCAL_MAXIMUM, status) if you really wanted to

Given we're indicating days of the week by 1 to 7 for weekend{Start,End}, directly corresponding to UCAL_DAY_OF_WEEK being specified -- stably -- as Sunday to Saturday -- it seems to me that we basically do.

I mean, I guess *theoretically* some calendar could consider the first week of the year as the first partial week and the first full week, or something weird.  But they don't that I'm aware of.  And if they did, consulting limits of UCAL_DAY_OF_WEEK would be inadequate to answer this question anyway, wouldn't it?

In any event, calling a fallible function to get a limit just for an assertion isn't worth it.  :-)  And we can live without an assertion if there's some theoretical reason it might conceivably ever be mistaken.

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for jsreftest bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=35073246&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/99b60f9205ef

Comment 26

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

Sorry for trouble Wes! Seems like jsreftests run in a browser don't have access to jsshell globals. I'll fix it

Comment 27

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3f97d528c6e5b6b47ccd0d9fc6476a5847dcae0
Bug 1287503 - Expose a JS Intl API to get calendar information. r=waldo

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f3f97d528c6e

Comment 29

[Scott Wu [:scottwu]]

Not sure if this is a bug for l10n nightly builds:

When I tested the date picker UI on nightly builds for different locales, the locale string returned from the JP version for mac (firefox-54.0a1.ja-JP-mac.mac.dmg) is "ja-JP-mac" (w/ chrome-registry), or "ja-JP-x-lvariant-mac" (w/ localeservice).

This causes an invalid language tag error in getCalendarInfo.

Comment 30

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

can you file it please as a bug? I'll investigate tomorrow.

Comment 31

[Scott Wu [:scottwu]]

Done! (Bug 1344141)
Thanks :zibi

Comment 32

[Scott Wu [:scottwu]]

My understanding is that "ja-JP-mac" is there to address the difference in wording btw operating systems, but it's not an acceptable language tag. I could fix it in my datepicker by manually mapping "ja-JP-mac" to "ja-JP", or do you think this should be done in the API implementation?

Comment 33

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

Filed bug 1346819 for this.

Comment 34

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

bug 1346819 is going to land tonight and will change the signature of getAppLocales - for all Intl/ICU purposes you'll want to use getAppLocalesAsBCP47.