Expose an API to get locale information

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla54
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Similarly to bug 1287503 for calendar, we should make it possible to retrieve information about locale directionality from Intl API.

There's some conversation about it on ECMA402, but we'll start with mozIntl:

let info = mozIntl.getLocaleInfo(locale);

info === {
  locale,
  dir // `ltr` or `rtl`
};

In the future we will expand this and add more bits.
(Assignee)

Updated

2 years ago
See Also: → bug 1312049
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1321004
(Assignee)

Updated

2 years ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1320265
Keywords: dev-doc-needed
(Assignee)

Updated

2 years ago
Blocks: 1280654

Comment 4

2 years ago
mozreview-review
Comment on attachment 8815358 [details]
Bug 1312053 - Expose an API to get locale information.

https://reviewboard.mozilla.org/r/96318/#review96516

::: js/src/builtin/Intl.cpp:109
(Diff revision 1)
>  {
>      MOZ_CRASH("Char16ToUChar: Intl API disabled");
>  }
>  
> +UBool
> +uloc_isRightToLeft(const char *locale) {

Brace on its own line and * immediately adjacent to the type, consistent with all the other code around it.

::: js/src/builtin/Intl.cpp:2945
(Diff revision 1)
> +
> +    RootedObject info(cx, NewBuiltinClassInstance<PlainObject>(cx));
> +    if (!info)
> +        return false;
> +
> +    bool rtl = uloc_isRightToLeft(icuLocale(locale.ptr()));

It seems problematic to consult ICU for this detail, when Gecko might consult a different source of truth that *potentially* could disagree.  Almost certainly, Gecko consults `layout/base/nsBidi_{,no}ICU.cpp` for this.  (Both versions exist because they're maintaining the two in parallel until ICU can be used everywhere.)  The ICU version is fine, but the non-ICU version is far more opaque what exactly it's using.  Someone who works on our bidi support, or maybe a Core::Internationalization person, needs to be brought in here to say exactly what Gecko uses, and if we -- hopefully temporarily -- need this to be something the embedder provides us by JSAPI callback.

This is certainly the biggest issue with this patch -- rest is mostly cosmetic.

::: js/src/builtin/Intl.cpp:2949
(Diff revision 1)
> +
> +    bool rtl = uloc_isRightToLeft(icuLocale(locale.ptr()));
> +
> +    RootedValue dirVal(cx);
> +
> +    dirVal = StringValue(JS_NewStringCopyZ(cx, rtl ? "rtl" : "ltr"));

Basically any JSAPI that takes `cx` as first argument is fallible, and if it returns falsy you should propagate the error.

But for constant strings like this, just add to CommonPropertyNames.h and use `cx->names().rtl` and such instead -- no need for a failure-check if you do that.

::: js/src/builtin/Intl.js:3025
(Diff revision 1)
> +                          localeOpt,
> +                          DateTimeFormat.relevantExtensionKeys,
> +                          localeData);
> +
> +  const result = intl_GetLocaleInfo(r.locale);
> +  result.locale = r.locale;

I think we prefer

    return { direction: intl_GetLocaleDirection(r.locale), locale: r.locale };

to doing the object creation/property-definition in C++.  Obviously the self-hosting intrinsic will need to be slightly different for this.

::: toolkit/components/mozintl/MozIntl.cpp:49
(Diff revision 1)
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +MozIntl::AddGetLocaleInfo(JS::Handle<JS::Value> val, JSContext* cx)
> +{

This'll need a somewhat different definition -- I commoned up a bunch of this code into one function that the various addFoo functions can all call.
Attachment #8815358 - Flags: review?(jwalden+bmo) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8815358 [details]
Bug 1312053 - Expose an API to get locale information.

https://reviewboard.mozilla.org/r/96318/#review96516

Updated to review. Don't bother with inter-diff :(

> It seems problematic to consult ICU for this detail, when Gecko might consult a different source of truth that *potentially* could disagree.  Almost certainly, Gecko consults `layout/base/nsBidi_{,no}ICU.cpp` for this.  (Both versions exist because they're maintaining the two in parallel until ICU can be used everywhere.)  The ICU version is fine, but the non-ICU version is far more opaque what exactly it's using.  Someone who works on our bidi support, or maybe a Core::Internationalization person, needs to be brought in here to say exactly what Gecko uses, and if we -- hopefully temporarily -- need this to be something the embedder provides us by JSAPI callback.
> 
> This is certainly the biggest issue with this patch -- rest is mostly cosmetic.

Good point, although I'm not sure if it's layout/base/nsBidi material. There's also nsChromeRegistry::getDirectionForLocale which is being moved to ICU (bug 1312049).

We're about to land ICU in Android, so I hope that this will stop being an issue, but I'll wait for that bug to get fixed.

> I think we prefer
> 
>     return { direction: intl_GetLocaleDirection(r.locale), locale: r.locale };
> 
> to doing the object creation/property-definition in C++.  Obviously the self-hosting intrinsic will need to be slightly different for this.

I aligned the code a bit with your feedback, but I kept the intl_GetLocaleInfo because we'll soon want to extend it to return more info (same as how intl_GetCalendarInfo works).

I'd like to avoid having to have separate intl_* function for each bit.

Let me know if it's ok, or do you need me to transform that.

Comment 7

2 years ago
mozreview-review
Comment on attachment 8815358 [details]
Bug 1312053 - Expose an API to get locale information.

https://reviewboard.mozilla.org/r/96318/#review108466

::: js/src/builtin/Intl.cpp:4417
(Diff revision 2)
> +
> +    RootedObject info(cx, NewBuiltinClassInstance<PlainObject>(cx));
> +    if (!info)
> +        return false;
> +
> +    bool rtl = uloc_isRightToLeft(icuLocale(locale.ptr()));

Ugh.  That's unfortunate there are three separate ways to ask this right now.  I guess as long as this only lands once ICU is the *only* way everyone consults, okay.  But I'm not happy about reviewing something conditional on something else landing first, when the dependency isn't directly obvious (like the implementation of some quite distant function needing to change).

::: js/src/builtin/Intl.js:3239
(Diff revision 2)
> +                          localeData);
> +
> +  const info = intl_GetLocaleInfo(r.locale);
> +  return {
> +      direction: info.direction,
> +      locale: r.locale

You're right that calling umpteen intrinsics to populate this isn't good.  But if those intrinsics wouldn't be used by anything but this one function, we should just group them all together into one call.  And if that's the case, we should probably just throw the "locale" property into the things generated by the one call -- and then this function would end with `return intl_GetLocaleInfo(r.locale);`.

::: toolkit/components/mozintl/MozIntl.cpp:92
(Diff revision 2)
> +{
> +  if (!val.isObject()) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
> +  JS::Rooted<JSObject*> realIntlObj(cx, js::CheckedUnwrap(&val.toObject()));

This isn't using the helper function I mentioned in the previous review.

::: toolkit/components/mozintl/test/test_mozintl.js:48
(Diff revision 2)
>  
>    mozIntl.addGetDisplayNames(x);
>    equal(x.getDisplayNames instanceof Function, true);
> +
> +  mozIntl.addGetLocaleInfo(x);
> +  equal(x.getDisplayNames instanceof Function, true);

Copy-paste error?
Attachment #8815358 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Comment 8

2 years ago
>  But I'm not happy about reviewing something conditional on something else landing first, when the dependency isn't directly obvious (like the implementation of some quite distant function needing to change).

Yeah, we're hopefully going to have ICU everywhere very soon and remove all non-ICU paths. I'll wait with this patch for that to happen.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8815358 - Attachment is obsolete: true
(Assignee)

Comment 10

2 years ago
mozreview-review-reply
Comment on attachment 8815358 [details]
Bug 1312053 - Expose an API to get locale information.

https://reviewboard.mozilla.org/r/96318/#review108466

> Ugh.  That's unfortunate there are three separate ways to ask this right now.  I guess as long as this only lands once ICU is the *only* way everyone consults, okay.  But I'm not happy about reviewing something conditional on something else landing first, when the dependency isn't directly obvious (like the implementation of some quite distant function needing to change).

with landing of bug 1312049 we now use ICU for rtl detection on all platforms that support INTL_API. Since mozIntl is also enabled only on those platforms I hope we'll be able to land it now.

Comment 11

2 years ago
mozreview-review
Comment on attachment 8833123 [details]
Bug 1312053 - Expose an API to get locale information.

https://reviewboard.mozilla.org/r/109348/#review113992

Mostly there, just a couple quasi-nits.

::: js/src/builtin/Intl.h:517
(Diff revision 1)
>  /**
> + * Returns a plain object with locale information for a single valid locale
> + * (callers must perform this validation).  The object will have these
> + * properties:
> + *
> + *   direction

You need to document the `locale` argument being added as a property -- and also mention the `locale` argument at all, actually.

::: js/src/builtin/Intl.cpp:4195
(Diff revision 1)
> +        return false;
> +
> +    RootedString localeStr(cx, JS_NewStringCopyZ(cx, locale.ptr()));
> +    RootedValue localeVal(cx, StringValue(localeStr));
> +
> +    if (!DefineProperty(cx, info, cx->names().locale, localeVal))

Uh, why are you copying an input string at all here?  Just pass `args[0]`.

::: js/src/builtin/Intl.cpp:4202
(Diff revision 1)
> +
> +    bool rtl = uloc_isRightToLeft(icuLocale(locale.ptr()));
> +
> +    RootedValue dirVal(cx);
> +
> +    dirVal = StringValue(rtl ? cx->names().rtl : cx->names().ltr);

RootedValue dir(cx, StringValue(rtl ? cx->names().rtl : cx->names().ltr));
Attachment #8833123 - Flags: review?(jwalden+bmo) → review-
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8833123 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8838822 [details]
Bug 1312053 - Expose an API to get locale information.

https://reviewboard.mozilla.org/r/113632/#review115728
Attachment #8838822 - Flags: review?(jwalden+bmo) → review+
Comment hidden (mozreview-request)

Comment 16

2 years ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/412c2a1e69da
Expose an API to get locale information. r=Waldo

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/412c2a1e69da
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.