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

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gwagner, Assigned: fabrice)

Tracking

Trunk
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

6 years ago
Blocks: 674720
(Reporter)

Updated

6 years ago
No longer blocks: 674720
(Reporter)

Updated

6 years ago
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: 733573
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.
(Assignee)

Comment 6

5 years ago
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.
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+]
(Assignee)

Comment 8

5 years ago
Created attachment 629010 [details] [diff] [review]
patch v2

Addressing comments.
Attachment #627400 - Attachment is obsolete: true
Attachment #629010 - Flags: review?(mrbkap)
(Assignee)

Comment 9

5 years ago
Created attachment 629266 [details] [diff] [review]
patch v3

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

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bcbb4ebcdd0
https://hg.mozilla.org/mozilla-central/rev/9bcbb4ebcdd0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
This introduced a hard tab... Please fix.
(Assignee)

Comment 14

5 years ago
(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
https://hg.mozilla.org/mozilla-central/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.
Blocks: 776027
You need to log in before you can comment on or make changes to this bug.