Closed Bug 723206 Opened 12 years ago Closed 12 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: gwagner, Assigned: fabrice)

References

Details

Attachments

(1 file, 2 obsolete files)

For Contacts we want something like "contact = new Contact({name : "Tom"});"
Right now only "contact = new Contact(); contact.init({name: "Tom"});" works.
Blocks: 674720
No longer blocks: 674720
Blocks: 674720
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
I think you can already do this by implementing nsIJSNativeInitializer in Contact.
Ah, hmm, that's non-scriptable. We could make it scriptable (we'd need to figure out what to do about the JSContext argument).
Can't it be done with something called nsIDOMGlobalConstructorInitializer similarly to nsIDOMGlobalPropertyInitializer from bug 610714? Or maybe this is overkill?
Blocks: 715814
(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).
"this" in my previous comment was to add a new scriptable interface, fwiw.
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → fabrice
Attachment #627400 - Flags: feedback?(mrbkap)
Whiteboard: [b2g:blocking+]
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.
Attachment #627400 - Flags: feedback?(mrbkap)
blocking-basecamp: --- → +
blocking-kilimanjaro: --- → +
Whiteboard: [b2g:blocking+]
Attached patch patch v2 (obsolete) — Splinter Review
Addressing comments.
Attachment #627400 - Attachment is obsolete: true
Attachment #629010 - Flags: review?(mrbkap)
Attached patch patch v3Splinter Review
Changing the way we resize the rooter array.
Attachment #629010 - Attachment is obsolete: true
Attachment #629010 - Flags: review?(mrbkap)
Attachment #629266 - Flags: review?(mrbkap)
Comment on attachment 629266 [details] [diff] [review]
patch v3

Looks good.
Attachment #629266 - Flags: review?(mrbkap) → review+
https://hg.mozilla.org/mozilla-central/rev/9bcbb4ebcdd0
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
This introduced a hard tab... Please fix.
(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
Geo: would you mind please finding a QA contact who could verify this?  Thanks!
Whiteboard: [qa+]
(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.)
Whiteboard: [qa+]
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?
Contact was never updated to use this feature; we should probably remove it again.
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.
Also, please don't remove this feature; we need it to implement the PeerConnection API.
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: