Last Comment Bug 723206 - Constructors implemented in JS from IDL should be allowed to have arguments
: Constructors implemented in JS from IDL should be allowed to have arguments
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: [:fabrice] Fabrice Desré
:
Mentors:
Depends on:
Blocks: 674720 715814 733573 776027
  Show dependency treegraph
 
Reported: 2012-02-01 11:03 PST by Gregor Wagner [:gwagner]
Modified: 2012-07-20 10:36 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
patch (5.57 KB, patch)
2012-05-25 15:58 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
patch v2 (4.13 KB, patch)
2012-05-31 17:41 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
patch v3 (4.17 KB, patch)
2012-06-01 11:23 PDT, [:fabrice] Fabrice Desré
mrbkap: review+
Details | Diff | Review

Description Gregor Wagner [:gwagner] 2012-02-01 11:03:58 PST
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] 2012-03-01 01:27:52 PST
I think you can already do this by implementing nsIJSNativeInitializer in Contact.
Comment 2 Peter Van der Beken [:peterv] 2012-03-01 01:31:31 PST
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) 2012-03-11 17:00:00 PDT
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) (please use needinfo!) 2012-05-15 10:06:50 PDT
(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) (please use needinfo!) 2012-05-15 10:07:43 PDT
"this" in my previous comment was to add a new scriptable interface, fwiw.
Comment 6 [:fabrice] Fabrice Desré 2012-05-25 15:58:24 PDT
Created attachment 627400 [details] [diff] [review]
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) (please use needinfo!) 2012-05-29 16:38:24 PDT
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é 2012-05-31 17:41:21 PDT
Created attachment 629010 [details] [diff] [review]
patch v2

Addressing comments.
Comment 9 [:fabrice] Fabrice Desré 2012-06-01 11:23:23 PDT
Created attachment 629266 [details] [diff] [review]
patch v3

Changing the way we resize the rooter array.
Comment 10 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-06-01 13:45:43 PDT
Comment on attachment 629266 [details] [diff] [review]
patch v3

Looks good.
Comment 11 [:fabrice] Fabrice Desré 2012-06-01 14:41:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bcbb4ebcdd0
Comment 12 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-02 12:13:12 PDT
https://hg.mozilla.org/mozilla-central/rev/9bcbb4ebcdd0
Comment 13 :Ms2ger 2012-06-02 13:39:34 PDT
This introduced a hard tab... Please fix.
Comment 14 [:fabrice] Fabrice Desré 2012-06-02 14:21:46 PDT
(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) 2012-06-03 12:31:49 PDT
https://hg.mozilla.org/mozilla-central/rev/a426ee764e00
Comment 16 Stephen Donner [:stephend] 2012-06-25 16:19:41 PDT
Geo: would you mind please finding a QA contact who could verify this?  Thanks!
Comment 17 Stephen Donner [:stephend] 2012-06-25 16:58:57 PDT
(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] 2012-07-19 01:23:35 PDT
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 2012-07-19 05:26:40 PDT
Contact was never updated to use this feature; we should probably remove it again.
Comment 20 Anant Narayanan [:anant] 2012-07-19 10:24:11 PDT
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] 2012-07-19 10:39:21 PDT
Also, please don't remove this feature; we need it to implement the PeerConnection API.
Comment 22 Anant Narayanan [:anant] 2012-07-19 10:45:21 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.