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

RESOLVED FIXED in Firefox 51

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

(Blocks 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 8 obsolete attachments)

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
(Assignee)

Updated

3 years ago
Assignee: nobody → gandalf
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)?
(Assignee)

Comment 2

3 years ago
Posted patch patch v1 (obsolete) — Splinter Review
: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.
Attachment #8775410 - Flags: feedback?(jwalden+bmo)
(Assignee)

Comment 3

3 years ago
Tim, :waldo suggested the approach similar to ctypes and I'm happy to use it.
(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 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.
Attachment #8775410 - Flags: feedback?(jwalden+bmo) → feedback+
(Assignee)

Updated

3 years ago
Blocks: 1270146
Comment hidden (obsolete)
(Assignee)

Comment 7

3 years ago
Posted patch patch v2 (obsolete) — Splinter Review
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
Attachment #8775453 - Attachment is obsolete: true
Attachment #8775453 - Flags: review?(jwalden+bmo)
Attachment #8775473 - Flags: review?(l10n)
(Assignee)

Updated

3 years ago
No longer blocks: 1287503
Depends on: 1287503
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.
Attachment #8775473 - Flags: review?(l10n)
I wondered, does this allow us to use these APIs in content l20n? about:robots, video xbl, etc?
(Assignee)

Comment 10

3 years ago
(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.
(Assignee)

Comment 11

3 years ago
Posted patch patch v3 (obsolete) — Splinter Review
: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.
Attachment #8775473 - Attachment is obsolete: true
Attachment #8775729 - Flags: review?(bzbarsky)
Attachment #8775729 - Flags: feedback?(jwalden+bmo)
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.
Attachment #8775729 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 13

3 years ago
Posted patch patch v4 (obsolete) — Splinter Review
isolating the toolkit patch, with bz's feedback applied, carrying over r+, asking for feedback from :waldo, and looking for a toolkit reviewer :)
Attachment #8775729 - Attachment is obsolete: true
Attachment #8775729 - Flags: feedback?(jwalden+bmo)
Attachment #8775759 - Flags: review+
Attachment #8775759 - Flags: feedback?(jwalden+bmo)
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.
(Assignee)

Comment 15

3 years ago
Posted patch patch v5 (obsolete) — Splinter Review
:mossop promised to take a look or find me a reviewer!

(addressed the indentation - 2 spaces per :mossop's advice)
Attachment #8775759 - Attachment is obsolete: true
Attachment #8775759 - Flags: feedback?(jwalden+bmo)
Attachment #8775780 - Flags: review+
Attachment #8775780 - Flags: feedback?(jwalden+bmo)
(Assignee)

Updated

3 years ago
Flags: needinfo?(dtownsend)
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
(Assignee)

Comment 17

3 years ago
Posted patch patch v6 (obsolete) — Splinter Review
Updated patch with feedback from :mossop
Attachment #8775780 - Attachment is obsolete: true
Attachment #8775780 - Flags: feedback?(jwalden+bmo)
Attachment #8775826 - Flags: review+
Attachment #8775826 - Flags: feedback?(jwalden+bmo)
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.
Attachment #8775826 - Flags: feedback?(jwalden+bmo) → feedback+
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
Attachment #8775826 - Flags: review+
Flags: needinfo?(dtownsend)
(Assignee)

Comment 20

3 years ago
Posted patch patch v7 (obsolete) — Splinter Review
Updated names to :waldo's feedback. Requesting review from :waldo.
Attachment #8775826 - Attachment is obsolete: true
Attachment #8775889 - Flags: review?(jwalden+bmo)
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);
}
Attachment #8775889 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 22

3 years ago
Posted patch patch v8Splinter Review
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.
Attachment #8775889 - Attachment is obsolete: true
Attachment #8777168 - Flags: review+

Updated

3 years ago
Priority: -- → P3
(Assignee)

Comment 26

3 years ago
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 :(
Flags: needinfo?(gandalf) → needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend)

Comment 28

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bef3532d2dc5
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 30

3 years ago
Pushed to inbound with :mossop's fix (which fixed it in a try build - https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e8748314edb )
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 33

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

Comment 35

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cc3a2d0ab8f9
https://hg.mozilla.org/mozilla-central/rev/ba64432bc2d1
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Depends on: 1300395
Blocks: 1332792
You need to log in before you can comment on or make changes to this bug.