Change the mozIntl API to hold target functions/classes instead of methods to add them

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: zbraniecki, Assigned: zbraniecki)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

Currently toolkit's mozIntl API has a set of methods to add additional/extended APIs from SM's Intl API. That works well as it keeps us close to ECMA402.

The way it works now is:

```
const mozIntlSvc = Components.classes["@mozilla.org/mozintl;1"].getService(Components.interfaces.mozIMozIntl);

const mozIntl = {};

mozIntlSvc.addPluralRulesConstructor(mozIntl);
mozIntlSvc.addGetCalendarInfo(mozIntl);
mozIntlSvc.addGetDisplayNames(mozIntl);

let pr = new mozIntl.PluralRules();
let data = mozIntl.getCalendarInfo();
let names = mozIntl.getDisplayNames();

// and soon:

mozIntlSvc.addGetLocaleInfo(mozIntl);
mozIntlSvc.addRelativeTimeFormatConstructor(mozIntl);
mozIntlSvc.addDateTimeFormatConstructor(mozIntl); // that's like Intl.DateTimeFormat with extra options
```

:smaug on IRC suggested that it would be easier if we morphed it into:

```
const mozIntl = Components.classes["@mozilla.org/mozintl;1"].getService(Components.interfaces.mozIMozIntl);

let pr = new mozIntl.PluralRules();
let data = mozIntl.getCalendarInfo();
let names = mozIntl.getDisplayNames();
```
:waldo, :smaug, :jfkthame - opinions?
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jfkthame)
Flags: needinfo?(bugs)
The API is super weird, and very difficult to use from C++.
We'll need something like what I proposed anyhow if we want to use the API in native anonymous XBL content (for date/time <input> element).
Flags: needinfo?(bugs)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #1)
> :waldo, :smaug, :jfkthame - opinions?

I'm the wrong person to ask. :) I don't really know enough about how JS APIs are managed to have a meaningful opinion here; it's entirely possible that I am misunderstanding key issues.

With that caveat, one thought: IIUC, the current mozIntl allows you to add the "extension" methods to the global Intl object. And thereafter, a developer (e.g. of FF front-end code) could just use Intl as if all the added capabilities were part of the standard (as we expect they eventually will be, at least in some cases, right?)

Whereas with the proposed change, the developer will often be dealing with two objects, Intl for the standard features and mozIntl for the extensions, having to remember which features each one implements. (And over time, things may move from mozIntl to Intl, requiring updates to client code.)

Having said that, :smaug's suggestion looks like it could make the simplest kinds of usage simpler.
Flags: needinfo?(jfkthame)
> With that caveat, one thought: IIUC, the current mozIntl allows you to add the "extension" methods to the global Intl object. And thereafter, a developer (e.g. of FF front-end code) could just use Intl as if all the added capabilities were part of the standard (as we expect they eventually will be, at least in some cases, right?)

We expect all of mozIntl features to end up in ECMA402 and Intl, yes.

> Whereas with the proposed change, the developer will often be dealing with two objects, Intl for the standard features and mozIntl for the extensions, having to remember which features each one implements. (And over time, things may move from mozIntl to Intl, requiring updates to client code.)

Which may be a good or a bad thing depending on our design approach. Migrating between Intl and mozIntl is rather easy, and having two different objects allows us to avoid confusion where Intl.DateTimeFormat may be overloaded with mozIntl functionality and it's not instantly visible.

This way mozIntl.DateTimeFormat is clearly different from Intl.DateTimeFormat and we can easily grep for what uses mozIntl and where.
It's important for eventual unconditional enabling in SpiderMonkey for these functions to be JSNatives -- because that's what the JS engine uses for function types.  Having functions defined by XPCOM, that can't simply/easily call a JSNative implementation of the relevant functionality, is an impedance mismatch.  And

  let pr = new mozIntl.PluralRules(...);

(and similar) all but requires that this PluralRules function be XPCOM-ful.  That would modify the exact stepwise algorithm PluralRules currently implements: not cool for functions *intended* not to be XPCOM forever.

Make whatever changes you want to the XPCOM interface/service/whatevers: I don't think I care.  But whatever happens, the PluralRules/getCalendarInfo/etc. functions must not go through XPCOM-like layering.  Comment 0's API almost surely doesn't do that.

On the other hand, something like

   let PluralRules = mozIntl.getPluralRules(); // the Intl.PluralRules ctor
   let getCalendarInfo = mozIntl.getGetCalendarInfo(); // Intl.getCalendarInfo

evades the problem, by having getPluralRules/getGetCalendarInfo be XPCOM.  They could return plain old JS functions that could be backed by JSNative, without XPCOM in sight.

But in the end, to be honest, it all seems like rearranging deckchairs to me.  I'd leave things as they are if I were deciding.
Flags: needinfo?(jwalden+bmo)
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment on attachment 8842671 [details]
Bug 1339892 - Refactor mozIntl to have a nicer API and thin logic.

We discussed it today with :smaug online and he suggested this approach.

It'll be useful for me to be able to inject the default locale from LocaleService and some more logic for MozDateTimeFormat to use OSPreferences.

Also, it gives nicer API.
Attachment #8842671 - Flags: feedback?(bugs)
CC'ing :scottwu, who I think will like the change :)
Ok, I think this is ready for review.
Comment on attachment 8842671 [details]
Bug 1339892 - Refactor mozIntl to have a nicer API and thin logic.

https://reviewboard.mozilla.org/r/116434/#review118134

::: toolkit/components/mozintl/mozIntl.js:42
(Diff revision 4)
> +    return this._cache.getCalendarInfo(getLocales(locales), ...args);
> +  }
> +
> +  getDisplayNames(locales, ...args) {
> +    if (!this._cache.hasOwnProperty("getDisplayNames")) {
> +      mozIntlHelper.addDisplayNames(this._cache);

Sorry to jump in. I was rebasing my patch based on this and found this error. I think this should be:
mozIntlHelper.addGetDisplayNames();

::: toolkit/components/mozintl/mozIntl.js:45
(Diff revision 4)
> +  getDisplayNames(locales, ...args) {
> +    if (!this._cache.hasOwnProperty("getDisplayNames")) {
> +      mozIntlHelper.addDisplayNames(this._cache);
> +    }
> +
> +    return this._cache.getDisplayNames(getLocales(locales), ...args);

getDisplayNames() may throw an exception but .idl does not support throwing exceptions. Maybe we should catch it and return an nsresult error value instead?
Comment on attachment 8842671 [details]
Bug 1339892 - Refactor mozIntl to have a nicer API and thin logic.

https://reviewboard.mozilla.org/r/116434/#review118134

> getDisplayNames() may throw an exception but .idl does not support throwing exceptions. Maybe we should catch it and return an nsresult error value instead?

Oh, but I get NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS as the result, so maybe it's ok.
Comment on attachment 8842671 [details]
Bug 1339892 - Refactor mozIntl to have a nicer API and thin logic.

https://reviewboard.mozilla.org/r/116434/#review118134

> Sorry to jump in. I was rebasing my patch based on this and found this error. I think this should be:
> mozIntlHelper.addGetDisplayNames();

Hah, thank you for the drive-by review :) I fixed it and added a test for it.

> Oh, but I get NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS as the result, so maybe it's ok.

I think I'd prefer not to do more than thin-wrap the function. I'm ok with bubbling up exceptions here.
Duplicate of this bug: 1334234
Comment on attachment 8842671 [details]
Bug 1339892 - Refactor mozIntl to have a nicer API and thin logic.

https://reviewboard.mozilla.org/r/116434/#review118334

::: toolkit/components/mozintl/MozIntlHelper.cpp:11
(Diff revision 5)
> -#include "MozIntl.h"
> +#include "MozIntlHelper.h"
>  #include "jswrapper.h"
>  #include "mozilla/ModuleUtils.h"
>  
> -#define MOZ_MOZINTL_CID \
> +#define MOZ_MOZINTLHELPER_CID \
>    { 0x83f8f991, 0x6b81, 0x4dd8, { 0xa0, 0x93, 0x72, 0x0b, 0xfb, 0x67, 0x4d, 0x38 } }

Not sure if it would be safer to update CID.

/msg firebot cid
on irc

::: toolkit/components/mozintl/mozIMozIntlHelper.idl:10
(Diff revision 5)
>  
>  #include "nsISupports.idl"
>  
> +/**
> + * This is an internal helper for mozIMozIntl API. There should be virtually
> + * no reason for you to call this API unless from mozIMozIntl implementation.

maybe s/unless/except/

::: toolkit/components/mozintl/mozIMozIntlHelper.idl:16
(Diff revision 5)
> + *
> + * This API helps accessing the SpiderMonkey Intl APIs, but it is mozIMozIntl
> + * that exposes the thin wrapper around them that binds the functionality
> + * to Gecko.
> + */
>  [scriptable, uuid(9f9bc42e-54f4-11e6-9aed-4b1429ac0ba0)]

could you update uuid here, just in case...better to not use the old uuid for the new interface
/msg firebot uuid

::: toolkit/components/mozintl/mozIntl.manifest:1
(Diff revision 5)
> +component {dd0bfbd0-1417-4495-9e41-f9051bffd591} mozIntl.js

Looks like you're using same uuid for the component as you're using for the interface.
Could you create another uuid for mozIMozIntl interface.

::: toolkit/components/mozintl/test/test_mozintlhelper.js:1
(Diff revision 5)
> +/* Any copyright is dedicated to the Public Domain.

So this is the old testfile? You should have just 'hg mv' to keep the annotation, and then perhaps
hg cp to create the new testfile for the new mozIntl interface
Attachment #8842671 - Flags: review?(bugs) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/afd35703e5b0
Refactor mozIntl to have a nicer API and thin logic. r=smaug
https://hg.mozilla.org/mozilla-central/rev/afd35703e5b0
https://hg.mozilla.org/mozilla-central/rev/f7fe1818b01e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.