723206 - Constructors implemented in JS from IDL should be allowed to have arguments

Status: RESOLVED FIXED

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

Description

[Gregor Wagner [:gwagner]]

For Contacts we want something like "contact = new Contact({name : "Tom"});"
Right now only "contact = new Contact(); contact.init({name: "Tom"});" works.

Comment 1

[Peter Van der Beken [:peterv]]

I think you can already do this by implementing nsIJSNativeInitializer in Contact.

Comment 2

[Peter Van der Beken [:peterv]]

Ah, hmm, that's non-scriptable. We could make it scriptable (we'd need to figure out what to do about the JSContext argument).

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Can't it be done with something called nsIDOMGlobalConstructorInitializer similarly to nsIDOMGlobalPropertyInitializer from bug 610714? Or maybe this is overkill?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Peter Van der Beken [:peterv] from comment #2)
> Ah, hmm, that's non-scriptable. We could make it scriptable (we'd need to
> figure out what to do about the JSContext argument).

We could remove it and require all implementers to use the context stack, but that seems really ugly. When I went to hack this up, though, I ran into a bug in our idl parser/generator (bug 755362).

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

"this" in my previous comment was to add a new scriptable interface, fwiw.

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: patch

Blake,

Just look at nsDOMClassInfo.cpp and the IDL.

The JS component is used for testing, along with http://people.mozilla.org/~fdesre/test - I'll factor this out as a test.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 627400 [details] [diff] [review]
patch

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

A few comments to fix here. Nothing major though.

::: dom/base/nsDOMClassInfo.cpp
@@ +5678,5 @@
> +        return NS_ERROR_FAILURE;
> +      }
> +
> +      jsval thisValue = JSVAL_VOID;
> +      JS::Value funval;

Make both of these JS::Values?

@@ +5690,5 @@
> +
> +      {
> +        JSAutoEnterCompartment tac;
> +
> +        JSObject* thisObject = JSVAL_TO_OBJECT(thisValue);

This could then be &thisValue.toObject();

@@ +5692,5 @@
> +        JSAutoEnterCompartment tac;
> +
> +        JSObject* thisObject = JSVAL_TO_OBJECT(thisValue);
> +
> +        if (!tac.enter(cx, thisObject) || !JS_WrapValue(cx, argv))

Did you mean to wrap thisValue here?

@@ +5697,5 @@
> +          return NS_ERROR_UNEXPECTED;
> +
> +        // wrap parameters in the target compartment
> +        for (size_t i = 0; i < argc; ++i) {
> +          if (!JS_WrapValue(cx, &argv[i]))

I know that I told you to do this, but you should probably create a new array on the heap to store these values, so we don't risk mixing the wrapped values.

@@ +5701,5 @@
> +          if (!JS_WrapValue(cx, &argv[i]))
> +            return NS_ERROR_FAILURE;
> +        }
> +
> +        JS_CallFunctionValue(cx, thisObject, funval, argc, argv, rval);

Need to check for failure here. I'd also use a fake rval (and throw if it isn't undefined) since we ignore the rval here below.

Comment 8

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Addressing comments.

Comment 9

[[:fabrice] Fabrice Desré]

Attachment: patch v3

Changing the way we resize the rooter array.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

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

Looks good.

Comment 11

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bcbb4ebcdd0

Comment 12

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/9bcbb4ebcdd0

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

This introduced a hard tab... Please fix.

Comment 14

[[:fabrice] Fabrice Desré]

(In reply to :Ms2ger from comment #13)
> This introduced a hard tab... Please fix.

ouch.. sorry:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a426ee764e00

Comment 15

[Phil Ringnalda (:philor)]

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

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

Geo: would you mind please finding a QA contact who could verify this?  Thanks!

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

(Clearing qa+, and my apologies for the spam.  We jumped the gun on some bug-verification process without understanding the full implications.  We'll follow up with a better method--shared with dev--for the next iteration.)

Comment 18

[Anant Narayanan [:anant]]

I'm unable to instantiate a Contact object via a constructor both in FF14 and the Nightly:

var c = new Contact();
TypeError: Contact is not a constructor

The Contact object itself is present in the global namespace. Am I doing something wrong, or has there been a regression?

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Contact was never updated to use this feature; we should probably remove it again.

Comment 20

[Anant Narayanan [:anant]]

The manifest registers Contact in the JavaScript-global-constructor category: https://mxr.mozilla.org/mozilla-central/source/dom/contacts/ContactManager.manifest#18 - is that not sufficient?

I'm trying to create another object (PeerConnection) that can take arguments as a global constructor, but run into the same error, so I'm trying to figure out if the feature works or if I'm doing something incorrectly.

Comment 21

[Anant Narayanan [:anant]]

Also, please don't remove this feature; we need it to implement the PeerConnection API.

Comment 22

[Anant Narayanan [:anant]]

Okay, sorry about the noise - vingtetun and gwagner clarified on IRC that mozContact is capable of taking arguments but the implementation hasn't changed yet to take advantage of the feature. I'll proceed to debug my code.