1289951 - Introduce a mozIntl component that will be exposing non-public JS Intl APIs

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P3)

Description

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

We have a need to start leveraging more of CLDR data and multiple JS Intl APIs are in the pipeline, but not ready to be exposed to the public.

The proposal is to create a mozIntl component that will work similarly to ctypes and expose JS Intl APIs to chrome code.

This will allow us to use them internally in Firefox and use the knowledge wew gain from that to refine the ECMA 402 APIs.

The first ones in the pipeline are:

* getCalendarInfo
* PluralRules
* getDisplayNames

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I agreed with this approach. Actually talked to Scott yesterday in office about this.

This is safer than blocking the feature on nailing down the public APIs.

I do have question on feasibility though; do our JS engine allow us to do this (exposing/hiding interfaces based on permission)?

Comment 2

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

Attachment: patch v1

:waldo, I got it working. Can you confirm that that's how you'd envision the code to look like?

I'll look for a reviewer in toolkit module.

Comment 3

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

Tim, :waldo suggested the approach similar to ctypes and I'm happy to use it.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #3)
> Tim, :waldo suggested the approach similar to ctypes and I'm happy to use it.

Nice! Thanks for helping out!

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8775410 [details] [diff] [review]
patch v1

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

::: toolkit/components/mozintl/moz.build
@@ +9,5 @@
> +]
> +
> +LOCAL_INCLUDES += [
> +    '/js/xpconnect/loader',
> +]

I think you might be able to compile without this addition -- if so, remove it.

::: toolkit/components/mozintl/mozintl.cpp
@@ +17,5 @@
> +  "@mozilla.org/jsmozintl;1"
> +
> +
> +#define JSMOZINTL_CID \
> +{ 0xc797701, 0x1c60, 0x4051, { 0x9a, 0xd7, 0x4d, 0x74, 0x5, 0x60, 0x56, 0x42 } }

This CID has been freshly generated, just to double-check?  I don't see any hits for c797701 in Searchfox, but gotta be sure.

@@ +36,5 @@
> +}
> +
> +#define XPC_MAP_CLASSNAME Module
> +#define XPC_MAP_QUOTED_CLASSNAME "Module"
> +#define XPC_MAP_WANT_CALL

There are other ways to invoke functionality defined by a module than making the module callable, but this probably works as well as anything.

@@ +42,5 @@
> +#include "xpc_map_end.h"
> +
> +
> +static bool
> +InitMozIntlClass(JSContext* cx, JS::Handle<JSObject*> global)

Probably name this AddMozIntlObject.  The other name style wasn't really even right for ctypes, tbh.

@@ +49,5 @@
> +  if (!mozIntl)
> +    return false;
> +
> +  static const JSFunctionSpec funcs[] = {
> +    JS_FS_END

You probably should submit this patch in a form that defines at least one function, so that it's not just a module that exports a plain object with nothing in it, to no obvious purpose.  :-)  Hopefully something will be ready soon enough for that to happen prior to a toolkit module review.

::: toolkit/components/mozintl/mozintl.jsm
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +this.EXPORTED_SYMBOLS = [ "mozIntl" ];
> +
> +// Initialize the ctypes object. You do not need to do this yourself.

Let's expand this out and have it document exactly what this module does:

"""
The following lines create a |mozIntl| object to be exported above by the module.  |mozIntl| is a plain object with a number of function properties.  Each function property should be called like so:

  Components.utils.import("resource://gre/modules/mozIntl.jsm");
  mozIntl.addPluralRulesConstructor(Intl);

passing in the global |Intl| object.  Calling a function adds an experimental, pre-standardized capability to the Intl object: plural-rules functionality, calendar information, and so on.  This allows the capability to be experimented with, while strictly cabining it to those few users who deliberately search out and invoke it.  Such users *must* be aware that experimental functionality can change at any time, and they commit to providing fallback behavior and updating their code as experimental capabilities are added, modified, and removed.
"""

And then, once experimental hooks are added, they should be documented in successive paragraphs or bullet points of this comment.

Comment 6

[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 7

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

Attachment: patch v2

I updated the patch, and chose 'getCalendarInfo' as the first target because the patch for it is closest to be complete.

I was unable to remove the xpconnect/loader. Without it, I don't have access to mozJSComponentLoader.h.

:Pike - you're listed as a peer for a Toolkit module[0] and you're familiar with our work to expose pre-standardized Intl APIs for Firefox to use.
Can you review this patch?

I'll land it only after we land the patch in bug 1287503.

[0] https://wiki.mozilla.org/Modules/Toolkit

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 8775473 [details] [diff] [review]
patch v2

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

This is very much out of my league, sorry. In particular the js-api stuff needs a reviewer that actually knows what those calls are doing.

Comment 9

[Axel Hecht [:Pike]]

I wondered, does this allow us to use these APIs in content l20n? about:robots, video xbl, etc?

Comment 10

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

(In reply to Axel Hecht [:Pike] from comment #9)
> I wondered, does this allow us to use these APIs in content l20n?
> about:robots, video xbl, etc?

No, it doesn't. That's the idea behind chrome vs content.

The content bindings will have to carry their own PluralRules until PluralRules become part of the standard :) Which I think will work because that way we'll maintain the 'content' variant which will be matching what the external web devs will be using.

Comment 11

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

Attachment: patch v3

:waldo - can you confirm that it matches your vision for the API?

:bz - can you review this pls? :)

It'll only land on top of patch from bug 1287503 which adds Intl_getCalendarInfo.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8775729 [details] [diff] [review]
patch v3

>+++ b/js/src/builtin/Intl.cpp
>+    MOZ_ASSERT(args.length() == 1);

I guess this is ok because this is never called with user-supplied values and only from self-hosted code?

>+    JSAutoByteString locale(cx, args[0].toString());

Similar here.

Just to make sure I understand the logic, weekendStartVal is supposed to be the first day of the weekend and weekendEndVal us the first day of the non-weekend?

Do we need to worry about cases when all days are type "weekened" or type "weekday"?  What about cases when the days of type "weekend" are non-contiguous (e.g. Wednesday and Friday)?  If not, document why not.

>+    return toClose.forget();

Please explain to me how this isn't leaking the UCalendar.  Shouldn't this be "return true"?

>+++ b/js/src/builtin/Intl.h

Since this function makes quite interesting assumptions about its arguments, those assumptions should be documented here.

>+++ b/js/src/builtin/Intl.js
>+  const r = ResolveLocale(callFunction(DateTimeFormat.availableLocales, DateTimeFormat),
>+                        requestedLocales,

Fix indent here, and on later lines.

>+  const result = intl_GetCalendarInfo(r.locale);

So r.locale is very guaranteed to be a string?  Enough that you're willing to risk doing weird things to memory if it's not?

>+++ b/js/src/shell/js.cpp
>+    JS::RootedObject intl(cx, &args.get(0).toObject());

This will get called by random shell consumers.  Do make sure to check args.get(0).isObject() and throw if not.

>+++ b/js/src/tests/Intl/getCalendarInfo.js

I did not review this in detail; I hope waldo will.

>+// |reftest| skip-if(!this.hasOwnProperty("Intl")||!addIntlExtras(Intl))

I don't get it.  addIntlExtras doesn't return a boolean.

>+++ b/toolkit/components/mozintl/nsMozIntl.cpp
>+  JS::Rooted<JSObject*> realIntlObj(cx, js::CheckedUnwrap(&val.toObject()));
>+	if (!realIntlObj) {

Please fix the indent.  And elsewhere in this function.

>+	if (!JS_DefineFunctions(cx, realIntlObj, funcs))
>+		return NS_OK;

Uh... no.  How can this be NS_OK, when things are clearly not OK?

You probably want return NS_ERROR_FAILURE or some such.  Doesn't matter too much which error code, but it needs to be an error code.

>+NS_GENERIC_FACTORY_CONSTRUCTOR(nsMozIntl)

This is a part that could use review from an existing toolkit peer.  It's not clear to me why this should be a new module instead of just being in whatever module toolkit has already.

>+namespace mozilla {
>+
>+class nsMozIntl

Having mozilla::nsMozIntl is a bit weird.  I'd probably just call this class "mozilla::MozIntl" without the ns.  Or even better mozilla::IntlExtensions or something, which is what it is.

r=me with the above issues addressed and a toolkit peer looking over the module bits.

Comment 13

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

Attachment: patch v4

isolating the toolkit patch, with bz's feedback applied, carrying over r+, asking for feedback from :waldo, and looking for a toolkit reviewer :)

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

This still has broken indentation in MozIntl::AddGetCalendarInfo....  And mixes up whether single-line if bodies are braced.  They should probably all be braced in this code.

Comment 15

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

Attachment: patch v5

:mossop promised to take a look or find me a reviewer!

(addressed the indentation - 2 spaces per :mossop's advice)

Comment 16

[Dave Townsend [:mossop]]

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

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

Do we have a test to verify this?

::: toolkit/components/mozintl/nsIMozIntl.idl
@@ +6,5 @@
> +#include "nsISupports.idl"
> +
> +%{C++
> +#define MOZINTL_CONTRACTID "@mozilla.org/mozintl;1"
> +%}

I don't see this used anywhere, is it needed?

::: toolkit/components/mozintl/nsMozIntl.cpp
@@ +21,5 @@
> +MozIntl::~MozIntl()
> +{
> +}
> +
> +  NS_IMETHODIMP

Incorrect indentation

Comment 17

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

Attachment: patch v6

Updated patch with feedback from :mossop

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 8775826 [details] [diff] [review]
patch v6

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

To be clear, this is f+ but not r+ just yet -- with the naming tweaks, and with the test modifications I'll get to you tomorrow it should be a simple r+.

::: toolkit/components/mozintl/moz.build
@@ +12,5 @@
> +
> +XPIDL_MODULE = 'mozintl'
> +
> +SOURCES += [
> +    'nsMozIntl.cpp',

I don't think nsMozIntl.{cpp,h} should have the "ns" prefix in the names.  Newer C++ files generally don't.

::: toolkit/components/mozintl/nsIMozIntl.idl
@@ +5,5 @@
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(9f9bc42e-54f4-11e6-9aed-4b1429ac0ba0)]
> +interface nsIMozIntl : nsISupports

mozIMozIntl, with the "moz" prefix, is also IMO mildly preferable to "ns".  Even if it doubles up on "moz".

::: toolkit/components/mozintl/test/test_mozintl.js
@@ +9,5 @@
> +
> +  mozIntl.addGetCalendarInfo(x);
> +
> +  let values = x.getCalendarInfo();
> +  ok(true);

I'd also like to see a test that goes across globals and across sandboxes, to exercise the JSAutoCompartment stuff that was added.  It's a tricky part of this that could be gotten wrong (was wrong, in my pastebin), and I'd rather not take it on blind faith.

I'll get back to you with the relevant test modifications tomorrow -- out of time for the day.  But the rest of the comments on this seem worth getting out there in advance of that, just in case you end up able to respond to these comments before I'd have posted a full review including test modifications.

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 8775826 [details] [diff] [review]
patch v6

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

Once Waldo is happy with the tests (and assuming it passes on try!) I'm happy for this to land

Comment 20

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

Attachment: patch v7

Updated names to :waldo's feedback. Requesting review from :waldo.

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 8775889 [details] [diff] [review]
patch v7

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

::: toolkit/components/mozintl/moz.build
@@ +12,5 @@
> +
> +XPIDL_MODULE = 'mozintl'
> +
> +SOURCES += [
> +    'mozIntl.cpp',

MozIntl.cpp for the name, with the "M" capitalized.

::: toolkit/components/mozintl/test/test_mozintl.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +function run_test() {

Assuming |is| is a function in tests these days (I dunno whether it is, I only remember do_check_eq and other such things, but the seeming-okay presence of |ok| suggests |is| probably exists), this should do what I want.  But please do test/try-run this -- I'm not 100% certain of the |test_cross_global| bits here.

function test_this_global(mozIntl) {
  let x = {};

  mozIntl.addGetCalendarInfo(x);
  is(x.getCalendarInfo instanceof Function, true);
  is(x.getCalendarInfo() instanceof Object, true);
}

function test_cross_global(mozIntl) {
  var global = new Components.utils.Sandbox("https://example.com/");
  var x = global.Object();

  mozIntl.addGetCalendarInfo(x);
  is(x.getCalendarInfo instanceof Function, false);
  is(x.getCalendarInfo instanceof global.Function, true);
  is(x.getCalendarInfo() instanceof Object, false);
  is(x.getCalendarInfo() instanceof global.Object, true);
}

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

  test_this_global(mozIntl);
  test_cross_global(mozIntl);

  ok(true);
}

Comment 22

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

Attachment: patch v8

Applied review feedback with two minor changes:

1) Used |equal| because |is| doesn't exist
2) Use Components.utils.waiveXrays to get access to the object.

I'll wait for bug 1287503 to land and then land this.

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac6a04f5bc7afb0a44410497e944d7a2b95cd81
Bug 1289951 - Add mozIntl toolkit component. r=waldo

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bef3532d2dc5
maybe needs a=CLOBBER CLOSED TREE

Comment 25

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/02c88cba5572 for the xpcshell bustage, which wasn't fixed by the clobber.

https://treeherder.mozilla.org/logviewer.html#?job_id=35160929&repo=mozilla-inbound

Comment 26

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

Dave, do you have any lead for me as how to debug this?

My local build works and the xpcshell test launched locally passes. I don't know why it doesn't on treeherder and I don't know how to debug it :(

Comment 27

[Dave Townsend [:mossop]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #26)
> Dave, do you have any lead for me as how to debug this?
> 
> My local build works and the xpcshell test launched locally passes. I don't
> know why it doesn't on treeherder and I don't know how to debug it :(

Your interface probably isn't being packaged. Updating https://dxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in to include mozintl.xpt will probably do it.

Comment 28

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

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

Comment 29

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c6d262e4b7f4e3ba8db75caed2468749aeb3f0
Bug 1289951 - Add mozIntl toolkit component. r=waldo

Comment 30

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

Pushed to inbound with :mossop's fix (which fixed it in a try build - https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e8748314edb )

Comment 31

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/81f39fe4a98b for Android failures, https://treeherder.mozilla.org/logviewer.html#?job_id=35265581&repo=mozilla-inbound

Comment 32

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc3a2d0ab8f9c3e1711ef7cc0093bed2b5af2ff1
Bug 1289951 - Add mozIntl toolkit component. r=waldo

Comment 33

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

Re-landed after try build ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff0d1ff8b18b ) with clean Android xpcshell tests.

Comment 34

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba64432bc2d1a4c3c0119c621e8fbd06b5abdd33
Follow-up to bug 1289951 - package mozIntl only if INTL is enabled. r=me

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/cc3a2d0ab8f9
https://hg.mozilla.org/mozilla-central/rev/ba64432bc2d1