Closed Bug 1312053 Opened 8 years ago Closed 7 years ago

Expose an API to get locale information

Categories

(Core :: JavaScript: Internationalization API, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Bug 1312053 - Expose an API to get locale information.
59 bytes, text/x-review-board-request
Waldo
: review+
Details 
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.
See Also: → 1312049
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
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 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 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-
>  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.
Attachment #8815358 - Attachment is obsolete: true
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 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-
Attachment #8833123 - Attachment is obsolete: true
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+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/412c2a1e69da
Expose an API to get locale information. r=Waldo
https://hg.mozilla.org/mozilla-central/rev/412c2a1e69da
Status: ASSIGNED → RESOLVED
Closed: 7 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.

Attachment

General

Creator:
Created:
Updated:
Size: