1341994 - Expose mozIntl.getDisplayNames to ChromeOrXBL

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jessica Jong [:jessica] (inactive)]

As suggested in bug 1301312 comment 19, we'd like to use mozIntl.getDisplayNames to get the localized strings for AM/PM, so we need to expose this API to ChromeOrXBL.

Comment 1

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

Olli, I have created a new interface `IntlUtils` for this, I think it'd be cleaner. What do you think? We can consider whether to move window.getAppLocales() to there too.

I made .getDisplayNames() a little bit simpler by accepting only a single key, cause I didn't find a good way to represent "an object of key-value pair list".

For more information about the API, see: http://searchfox.org/mozilla-central/rev/60ae6514e4c559c0c234f0e7aefccb101b8beb2e/js/src/builtin/Intl.js#3244

Comment 3

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

Sorry for an ignorant question, but... why do we need all this overhead for APIs that are for use in our chrome code?

The way I understood it is that XBL is only available in XUL/XHML which is only available in chrome context. What makes it impossible for us to just expose Components system in XBL?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

XBL in web pages doesn't run with chrome privileges, but with XBL privileges.
We use XBL to implement for example the internals of <audio> and <video>, and now also some date/time for <input>.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8840784 [details] [diff] [review]
patch, v2.

>+IntlUtils::GetMozIntlObj(JSContext* aCx, JS::MutableHandleObject aRetObject) {
Nit, { goes to its own line

>+  nsCOMPtr<mozIMozIntl> mozIntl = do_GetService("@mozilla.org/mozintl;1");
>+  if (!mozIntl) {
>+    return false;
>+  }
>+
>+  // Prepare random plain object for mozIntl.
>+  JS::RootedObject obj(aCx, JS_NewPlainObject(aCx));
>+  if (!obj) {
>+    return false;
>+  }
>+  JS::Rooted<JS::Value> objVal(aCx, JS::ObjectValue(*obj));
>+
>+  // Add 'getDisplayNames' method into the random plain object.
>+  mozIntl->AddGetDisplayNames(objVal, aCx);
>+
>+  aRetObject.set(js::CheckedUnwrap(&objVal.toObject()));
Why do we need to unwrap?


>+IntlUtils::GetPropertyFromJSObject(JSContext* aCx,
>+                                   JS::Handle<JSObject*> aObject,
>+                                   const char* aName, nsAString& aRetVal)
I hoping there isn't need for this but looking...

>+IntlUtils::GetDisplayNames(const Sequence<nsString>& aLocales,
>+                           const DisplayNameOptions& aOptions,
>+                           Nullable<mozilla::dom::DisplayNameResult>& aResult)
>+{
>+  AutoJSAPI jsapi;
>+  jsapi.Init();
>+  JSContext *cx = jsapi.cx();
>+  JSAutoCompartment ac(cx, GetWrapper());
So this uses web page scope. We don't want that.
Change the webidl so that we get JSContext as a param and just use that. It should be already in right compartment.
But please assert that it is in XBL or Chrome compartment.


>+  // Prepare parameter for getDisplayNames().
>+  JS::Rooted<JSObject*> locales(cx, JS_NewArrayObject(cx, aLocales.Length()));
>+  JS::Rooted<JSString*> str(cx);
>+  for (uint32_t i = 0; i < aLocales.Length(); ++i) {
>+    const nsString& locale = aLocales[i];
>+    str = JS_NewUCStringCopyZ(cx, locale.get());
>+    if (!str) {
>+      aResult.SetNull();
>+      return;
>+    }
>+    if (!JS_DefineElement(cx, locales, i, str, JSPROP_ENUMERATE)) {
>+      aResult.SetNull();
>+      return;
>+    }
Doesn't ToJSValue handle this kind of case.
http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/dom/bindings/ToJSValue.h#343-361

>+  }
>+
>+  JS::RootedObject optionsObj(cx, JS_NewPlainObject(cx));
>+  if (!optionsObj) {
>+    aResult.SetNull();
>+    return;
>+  }
>+
>+  if (aOptions.mStyle.WasPassed()) {
>+    const nsString& style = aOptions.mStyle.Value();
>+    JS::Rooted<JSString*> str(cx, JS_NewUCStringCopyN(cx,
>+                                                      style.get(),
>+                                                      style.Length()));
>+    JS::Rooted<JS::Value> styleVal(cx, JS::StringValue(str));
>+    if (!JS_SetProperty(cx, optionsObj, "style", styleVal)) {
>+      aResult.SetNull();
>+      return;
>+    }
>+  }
>+
>+  if (aOptions.mKey.WasPassed()) {
>+    const nsString& key = aOptions.mKey.Value();
>+    JS::Rooted<JSObject*> keysArrayObj(cx, JS_NewArrayObject(cx, 1));
>+    JS::Rooted<JSString*> str(cx, JS_NewUCStringCopyN(cx,
>+                                                      key.get(),
>+                                                      key.Length()));
>+
>+    if (!JS_DefineElement(cx, keysArrayObj, 0, str, JSPROP_ENUMERATE)) {
>+      aResult.SetNull();
>+      return;
>+    }
>+
>+    JS::Rooted<JS::Value> keysVal(cx, JS::ObjectValue(*keysArrayObj));
>+    if (!JS_SetProperty(cx, optionsObj, "keys", keysVal)) {
>+      aResult.SetNull();
>+      return;
>+    }
>+  }
Could we possibly make the webidl API to look more like the JS API and then use the bindings code to convert a dictionary to
JSValue.
So, DisplayNameOptions would need to have sequence<DOMString> keys I guess.


>+
>+  // Now call the method.
>+  JS::Rooted<JS::Value> retVal(cx);
>+  JS::AutoValueArray<2> args(cx);
>+  args[0].setObject(*locales);
>+  args[1].setObject(*optionsObj);
>+
>+  bool ok = JS_CallFunctionName(cx, hostObj, "getDisplayNames", args, &retVal);
>+  if (!ok) {
>+    aResult.SetNull();
>+    return;
>+  }
I wonder if we could even make webidl callback interface for this and let the generated code deal with this.
The callback interface would have method for getDisplayNames and its param + return value could be expressed in webidl.



>+
>+  // Unwrap the result and set it to the return value.
>+  JS::Rooted<JSObject*> retObj(cx, js::CheckedUnwrap(&retVal.toObject()));
>+
>+  nsAutoString locale;
>+  if (!GetPropertyFromJSObject(cx, retObj, "locale", locale)) {
>+    aResult.SetNull();
>+    return;
>+  }
>+
>+  nsAutoString style;
>+  if (!GetPropertyFromJSObject(cx, retObj, "style", style)) {
>+    aResult.SetNull();
>+    return;
>+  }
>+
>+  nsAutoString value;
>+  if (aOptions.mKey.WasPassed()) {
>+    // Get the value for the key-value pair in values.
>+    JS::Rooted<JS::Value> valuesVal(cx);
>+    if (!JS_GetProperty(cx, retObj, "values", &valuesVal)) {
>+      aResult.SetNull();
>+      return;
>+    }
>+    JS::Rooted<JSObject*> valuesObj(cx,
>+                                    js::CheckedUnwrap(&valuesVal.toObject()));
>+
>+    if (!GetPropertyFromJSObject(cx, valuesObj,
>+          NS_ConvertUTF16toUTF8(aOptions.mKey.Value()).get(), value)) {
>+      aResult.SetNull();
>+      return;
>+    }
>+  }

>+++ b/dom/tests/mochitest/general/test_interfaces.js
>@@ -600,16 +600,18 @@ var interfaceNamesInGlobalScope =
>     "InputEvent",
> // IMPORTANT: Do not change this list without review from a DOM peer!
>     "InstallTrigger",
> // IMPORTANT: Do not change this list without review from a DOM peer!
>     {name: "IntersectionObserver", disabled: true},
> // IMPORTANT: Do not change this list without review from a DOM peer!
>     {name: "IntersectionObserverEntry", disabled: true},
> // IMPORTANT: Do not change this list without review from a DOM peer!
>+    {name: "IntlUtils", xbl: true},
>+// IMPORTANT: Do not change this list without review from a DOM peer!
I don't think this is needed, if you have the [NoInterfaceObject]


>+[Func="IsChromeOrXBL"]
>+interface IntlUtils {
You don't need to expose the interface at all. So, [NoInterfaceObject].
you'll just create an instance of it and that can be used in chrome and XBL per the changes to Window interface.


>+
>+  /**
>+   * Getter funcion for IntlUtils, which provides helper functions for
function



So I think the JS part could be simplified quite a bit if we relied on webidl some more.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

oh, perhaps values can't be easily expressed as dictionary.
But if we use XBL scope here anyhow, what if the return value is just object, containing style and everything?

Comment 7

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #6)
> oh, perhaps values can't be easily expressed as dictionary.
> But if we use XBL scope here anyhow, what if the return value is just
> object, containing style and everything?

Using 'object' as the return value makes this a loooot easier :) Will change to return object instead then.


(In reply to Olli Pettay [:smaug] from comment #5)
> Comment on attachment 8840784 [details] [diff] [review]
> patch, v2.
> 
> >+IntlUtils::GetMozIntlObj(JSContext* aCx, JS::MutableHandleObject aRetObject) {
> Nit, { goes to its own line
> 
> >+  nsCOMPtr<mozIMozIntl> mozIntl = do_GetService("@mozilla.org/mozintl;1");
> >+  if (!mozIntl) {
> >+    return false;
> >+  }
> >+
> >+  // Prepare random plain object for mozIntl.
> >+  JS::RootedObject obj(aCx, JS_NewPlainObject(aCx));
> >+  if (!obj) {
> >+    return false;
> >+  }
> >+  JS::Rooted<JS::Value> objVal(aCx, JS::ObjectValue(*obj));
> >+
> >+  // Add 'getDisplayNames' method into the random plain object.
> >+  mozIntl->AddGetDisplayNames(objVal, aCx);
> >+
> >+  aRetObject.set(js::CheckedUnwrap(&objVal.toObject()));
> Why do we need to unwrap?
> 

Right, no need to unwrap.

> 
> >+IntlUtils::GetPropertyFromJSObject(JSContext* aCx,
> >+                                   JS::Handle<JSObject*> aObject,
> >+                                   const char* aName, nsAString& aRetVal)
> I hoping there isn't need for this but looking...
> 
> >+IntlUtils::GetDisplayNames(const Sequence<nsString>& aLocales,
> >+                           const DisplayNameOptions& aOptions,
> >+                           Nullable<mozilla::dom::DisplayNameResult>& aResult)
> >+{
> >+  AutoJSAPI jsapi;
> >+  jsapi.Init();
> >+  JSContext *cx = jsapi.cx();
> >+  JSAutoCompartment ac(cx, GetWrapper());
> So this uses web page scope. We don't want that.
> Change the webidl so that we get JSContext as a param and just use that. It
> should be already in right compartment.
> But please assert that it is in XBL or Chrome compartment.

Using 'object' will give us a JSContext* argument, will add the assertions.

> 
> 
> >+  // Prepare parameter for getDisplayNames().
> >+  JS::Rooted<JSObject*> locales(cx, JS_NewArrayObject(cx, aLocales.Length()));
> >+  JS::Rooted<JSString*> str(cx);
> >+  for (uint32_t i = 0; i < aLocales.Length(); ++i) {
> >+    const nsString& locale = aLocales[i];
> >+    str = JS_NewUCStringCopyZ(cx, locale.get());
> >+    if (!str) {
> >+      aResult.SetNull();
> >+      return;
> >+    }
> >+    if (!JS_DefineElement(cx, locales, i, str, JSPROP_ENUMERATE)) {
> >+      aResult.SetNull();
> >+      return;
> >+    }
> Doesn't ToJSValue handle this kind of case.
> http://searchfox.org/mozilla-central/rev/
> 31b6089ce26fa76459642765115605d50a6c67b4/dom/bindings/ToJSValue.h#343-361

Yes, thanks for this.

> 
> >+  }
> >+
> >+  JS::RootedObject optionsObj(cx, JS_NewPlainObject(cx));
> >+  if (!optionsObj) {
> >+    aResult.SetNull();
> >+    return;
> >+  }
> >+
> >+  if (aOptions.mStyle.WasPassed()) {
> >+    const nsString& style = aOptions.mStyle.Value();
> >+    JS::Rooted<JSString*> str(cx, JS_NewUCStringCopyN(cx,
> >+                                                      style.get(),
> >+                                                      style.Length()));
> >+    JS::Rooted<JS::Value> styleVal(cx, JS::StringValue(str));
> >+    if (!JS_SetProperty(cx, optionsObj, "style", styleVal)) {
> >+      aResult.SetNull();
> >+      return;
> >+    }
> >+  }
> >+
> >+  if (aOptions.mKey.WasPassed()) {
> >+    const nsString& key = aOptions.mKey.Value();
> >+    JS::Rooted<JSObject*> keysArrayObj(cx, JS_NewArrayObject(cx, 1));
> >+    JS::Rooted<JSString*> str(cx, JS_NewUCStringCopyN(cx,
> >+                                                      key.get(),
> >+                                                      key.Length()));
> >+
> >+    if (!JS_DefineElement(cx, keysArrayObj, 0, str, JSPROP_ENUMERATE)) {
> >+      aResult.SetNull();
> >+      return;
> >+    }
> >+
> >+    JS::Rooted<JS::Value> keysVal(cx, JS::ObjectValue(*keysArrayObj));
> >+    if (!JS_SetProperty(cx, optionsObj, "keys", keysVal)) {
> >+      aResult.SetNull();
> >+      return;
> >+    }
> >+  }
> Could we possibly make the webidl API to look more like the JS API and then
> use the bindings code to convert a dictionary to
> JSValue.
> So, DisplayNameOptions would need to have sequence<DOMString> keys I guess.

If using object as the return value, then we can accept multiple keys and convert the dictionary to JSValue directly.

> 
> 
> >+
> >+  // Now call the method.
> >+  JS::Rooted<JS::Value> retVal(cx);
> >+  JS::AutoValueArray<2> args(cx);
> >+  args[0].setObject(*locales);
> >+  args[1].setObject(*optionsObj);
> >+
> >+  bool ok = JS_CallFunctionName(cx, hostObj, "getDisplayNames", args, &retVal);
> >+  if (!ok) {
> >+    aResult.SetNull();
> >+    return;
> >+  }
> I wonder if we could even make webidl callback interface for this and let
> the generated code deal with this.
> The callback interface would have method for getDisplayNames and its param +
> return value could be expressed in webidl.

I think I'll just keep the way it is now, since we'll be able to access mozIntl directly after bug 1339892.

> 
> 
> 
> >+
> >+  // Unwrap the result and set it to the return value.
> >+  JS::Rooted<JSObject*> retObj(cx, js::CheckedUnwrap(&retVal.toObject()));
> >+
> >+  nsAutoString locale;
> >+  if (!GetPropertyFromJSObject(cx, retObj, "locale", locale)) {
> >+    aResult.SetNull();
> >+    return;
> >+  }
> >+
> >+  nsAutoString style;
> >+  if (!GetPropertyFromJSObject(cx, retObj, "style", style)) {
> >+    aResult.SetNull();
> >+    return;
> >+  }
> >+
> >+  nsAutoString value;
> >+  if (aOptions.mKey.WasPassed()) {
> >+    // Get the value for the key-value pair in values.
> >+    JS::Rooted<JS::Value> valuesVal(cx);
> >+    if (!JS_GetProperty(cx, retObj, "values", &valuesVal)) {
> >+      aResult.SetNull();
> >+      return;
> >+    }
> >+    JS::Rooted<JSObject*> valuesObj(cx,
> >+                                    js::CheckedUnwrap(&valuesVal.toObject()));
> >+
> >+    if (!GetPropertyFromJSObject(cx, valuesObj,
> >+          NS_ConvertUTF16toUTF8(aOptions.mKey.Value()).get(), value)) {
> >+      aResult.SetNull();
> >+      return;
> >+    }
> >+  }
> 
> >+++ b/dom/tests/mochitest/general/test_interfaces.js
> >@@ -600,16 +600,18 @@ var interfaceNamesInGlobalScope =
> >     "InputEvent",
> > // IMPORTANT: Do not change this list without review from a DOM peer!
> >     "InstallTrigger",
> > // IMPORTANT: Do not change this list without review from a DOM peer!
> >     {name: "IntersectionObserver", disabled: true},
> > // IMPORTANT: Do not change this list without review from a DOM peer!
> >     {name: "IntersectionObserverEntry", disabled: true},
> > // IMPORTANT: Do not change this list without review from a DOM peer!
> >+    {name: "IntlUtils", xbl: true},
> >+// IMPORTANT: Do not change this list without review from a DOM peer!
> I don't think this is needed, if you have the [NoInterfaceObject]
> 
> 
> >+[Func="IsChromeOrXBL"]
> >+interface IntlUtils {
> You don't need to expose the interface at all. So, [NoInterfaceObject].
> you'll just create an instance of it and that can be used in chrome and XBL
> per the changes to Window interface.

Right, will use the [NoInterfaceObject] annotation.

> 
> 
> >+
> >+  /**
> >+   * Getter funcion for IntlUtils, which provides helper functions for
> function
> 
> 
> 
> So I think the JS part could be simplified quite a bit if we relied on
> webidl some more.

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v3.

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: patch based on bug 1339892.

Comment 10

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8842784 [details] [diff] [review]
patch based on bug 1339892.

Attachment 8842766 [details] [diff] works fine, but with this, I get "Access to privileged JS object not permitted". Do we need to do something like Components.utils.cloneInto, but in cpp?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Actually, still another option, which I hope would work too, and would be in a way safer since it would reduce JS API usage:
Could the returned dictionary use record<DOMString, DOMString> for the values.
(Sorry, I forgot about record<>, that is very new addition to webidl.
Based on http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/dom/bindings/test/TestCodeGen.webidl#1125 it should work)

Something like

dictionary DisplayNameOptions {
  DOMString style;
  sequence<DOMString> keys;
};

dictionary DisplayNameResult {
  DOMString locale;
  DOMString style;
  record<DOMString, DOMString> values;
};

[NoInterfaceObject]
interface IntlUtils {
  DisplayNameResult? getDisplayNames(sequence<DOMString> locales,
                                     optional DisplayNameOptions options);




The patch on top of bug 1339892 would indeed need some changes. That bug makes mozIntl implementation to use chrome JS, and passing XBL JS there shouldn't work, nor passing chrome JS result back to XBL JS.
One may need to enter PrivilegedJunkScope()'s compartment to deal with mozIntl

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v4 (using mozIMozIntlHelper).

Comment 13

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8843177 [details] [diff] [review]
patch, v4 (using mozIMozIntlHelper).

This patch uses record<DOMString, DOMString> in DisplayNameResult, but still uses mozIMozIntlHelper. Using mozIMozIntl directly, I get a 'compartment mismatch' when retrieving the result from GetDisplayNames(), even using PrivilegedJunkScope(), maybe I'm doing something wrong here... but I wonder if it's a goog approach entering privileged junk scope?

Comment 14

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v4.1 (using mozIMozIntl).

Comment 15

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8843262 [details] [diff] [review]
patch, v4.1 (using mozIMozIntl).

I made it work using mozIMozIntl directly, leaving to you to decide which approach is better. :)

Comment 16

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

I'd prefer you to use mozIntl unless there's a performance win from using the Helper.

I'd like to keep the Helper "private" for mozIntl, and mozIntl API "public". The additional value is that all the thin-wrapper operations like selecting the default locale or datetime formatting will happen in mozIntl.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8843262 [details] [diff] [review]
patch, v4.1 (using mozIMozIntl).

>+IntlUtils::GetDisplayNames(JSContext* aCx, const Sequence<nsString>& aLocales,
We shouldn't need aCx when relying on non any/object webidl values

>+                           const DisplayNameOptions& aOptions,
>+                           DisplayNameResult& aResult, ErrorResult& aError)
>+{
>+  MOZ_ASSERT(nsContentUtils::IsCallerChrome() ||
>+             nsContentUtils::IsCallerContentXBL());
>+
>+  nsCOMPtr<mozIMozIntl> mozIntl = do_GetService("@mozilla.org/mozintl;1");
>+  if (!mozIntl) {
>+    aError.Throw(NS_ERROR_UNEXPECTED);
>+    return;
>+  }
>+
>+  aError.MightThrowJSException();
>+
>+  // Need to enter priviliged junk scope since mozIntl implementation is in
privileged

>+  // Now call the method.
>+  JS::Rooted<JS::Value> retVal(pjsCx);
>+  nsresult rv = mozIntl->GetDisplayNames(locales, options, &retVal);
>+  if (NS_FAILED(rv)) {
>+    aError.Throw(rv);
>+  }
missing return; after Throw

>+
>+  // Switch back to caller's compartment.
>+  JS::Rooted<JS::Value> jsValue(aCx);
>+  if (!ToJSValue(aCx, retVal, &jsValue)) {
>+    aError.StealExceptionFromJSContext(pjsCx);
>+    return;
>+  }
>+
>+  // Return the result as DisplayNameResult.
>+  if (!aResult.Init(aCx, jsValue)) {
>+     aError.Throw(NS_ERROR_FAILURE);
>+  }
Why do we need to switch back to callers compartment?
Couldn't we just pass pjsCx and retVal to Init?
If we rely on webidl bindings, we don't even need the aCx param

Something like the following should work without need for aCx
  if (!retVal.isObject()) {
    aError.Throw(NS_ERROR_FAILURE);
    return;
  }

  // Return the result as DisplayNameResult.
  JSAutoCompartment ac(jsapi.cx(), &retVal.toObject());
  if (!aResult.Init(jsapi.cx(), retVal)) {
    aError.Throw(NS_ERROR_FAILURE);
  }


>+
>+'IntlUtils': {
>+    'implicitJSContext': [ 'getDisplayNames' ],
>+},
So I think this was needed only when returning object.


>+   *   style:
>+   *     negotiated style
>+   *
>+   *   value:
values

Comment 18

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v5.

Thanks Olli, addressed your review comment 17.

Comment 19

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=15bd2a61171be8d8bb7703392a1694ad82462996&group_state=expanded

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75e73a2476b8
Expose mozIntl.getDisplayNames to ChromeOrXBL. r=smaug

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/75e73a2476b8