Presumably this will require defining some standard init functions on the JS implementation and invoking that after we run the standard constructor boilerplate. I think the navigator registration stuff must deal with this, so I should look at what it does.

The right way to do this is probably adding some kind of init method to the inner callback interface that isn't added to the outer binding. Then call the init method after the callback interface object is built.

Yes, I agree. That's basically what I was thinking as well. we can call the method _init or something like that.

Contacts needs this for mozContact.

Hitting this now in bug 823512 (already linked).

I'll work on this after I finish bug 851639.

Great! This is the last real blocker to something in bug 823512 we can turn on.

Kind of hacky, but it seems to work. It will only work on things with a single constructor. When a constructor is invoked, the arguments will be passed to method named __init(). I was going to thread the arguments through the constructors for the inner and outer wrappers, but that was annoying and I wasn't really sure how to deal with errors, so I just added a separate method to the callback interface, and invoke the directly in Constructor().

Does this replace nsIDOMGlobalPropertyInitializer? My __init function is not getting called... Investigating.

Here's the example I used for testing, if that helps. > Does this replace nsIDOMGlobalPropertyInitializer? Rather lamely, no. :) When we build an object, we first call init with the window (if it QIs to GPI), then call __init with the constructor arguments. Eventually it would be good to combine these somehow...

Could we just always call init with window and arguments?

Unbitrotted and changed with bz's help to work with cx argument which got added over the weekend.

I resolved my initial problem which was pilot error btw. Thanks for your patience there. The remaining issue now is that I need to call initEvent from __init, which doesn't work: __init: function(type, eventInitDict) { this.__DOM_IMPL__.initEvent(type, false, false); ... }, ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "'[JavaScript Error: "this.__DOM_IMPL__ is undefined" {file: "file:///.../components/PeerConnection.js" line: 232}]' when calling method: [IPeerConnectionObserver::onSetLocalDescriptionSuccess]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0" data: yes] ************************************************************ Bz says __DOM_IMPL__ isn't created yet, so there's an ordering issue here. [1:12pm] bz: So the current ordering of stuff is [1:13pm] bz: 1) Create chrome-side object [1:13pm] bz: 2) Create C++ object wrapping chrome-side JS object [1:13pm] bz: 3) Create C++ object for content wrapping C++ thing from step 2 [1:13pm] bz: 4) Create content-side JS object [1:13pm] bz: DOM_IMPL is set at step 4 [1:13pm] bz: mccr8 inserted the __init call between steps 1 and 2 I'm working around this with a manual init3() function at the moment, in the interest of landing this patch hopefully this week and land bug 823512 before FF23 uplift. This should be ok as PeerConnection is firing all the events.

> I resolved my initial problem which was pilot error btw. Thanks for your patience there. Great! I'm glad to hear it isn't something wrong with the patch. ;) > so there's an ordering issue here. Ah, very cool! I didn't even think about that aspect of it. I guess I haven't mentally internalized the weird thing where you reach to your outer wrapper from the JS impl. I'll fix that today, then work on getting it in reviewable state.

> Could we just always call init with window and arguments? Yeah, eventually I'd like to just have __init take window in addition to any constructor arguments, and remove the property initializer thing. DOM things that don't have args and don't care about window would have to write a trivial __init anyways, but that's probably a corner case not worth optimizing. I may just file a followup on that. One thing I'm not sure how to deal with is multiple constructors with arguments, because they'd all result in calls to a single __init method which is not really going to work in general. I'll probably just throw for now (which my test code does), if nobody has an idea on how to simply implement that.

Why are multiple constructors with arguments a problem, per se? Maybe I'm just missing something....

If you had a constructor that takes one argument, it would call __init(arg1) and if you had another that took two then it would call __init(arg2, arg3). Is there a reasonable way to implement them both using a single JS function?

Sure. It works just like this JS: function foo(arg1, arg2, arg3) { // Start examining our arguments } foo(5); foo("abc", new Object()); foo(new Object(), document, undefined);

This is trickier than I thought at a glance, because the wrap call happens in _constructor not Constructor. bz's suggestion was to sink the wrap into Constructor, which would let us move the __Init call after it. I'll try that tomorrow.

I cleaned up the code a bit, but nothing substantial.

This stuff is all wrapper-cached. So you can do a wrap in Constructor, and then in _constructor it will find the cached wrapper, right?

Ah, good point! So I should be able to just copy pasta some code into Constructor then ignore _constructor.

This should set __DOM_IMPL__ before the call to __Init(). I tried it out, and it seemed to work. I set an expando on __DOM_IMPL__ in Init() that was invisible from content and visible from chrome, but that's probably expected.

Does this work for you, jib?

> but that's probably expected. Yes, absolutely.

Works! Awesome.

Comment on attachment 746598 [details] [diff] [review] v4 + trivial test >+ initCall = "" This could go in an else branch, right? >+ constructorArgs = [arg.name for arg in self.getArgs(self.signature[0], self.signature[1])[2:]] This needs two things: 1) Documentation about why you're slicing off the first two arguments. 2) Assertions about what those arguments are like. See the assertions CGCallback.getMethodImpls has, for example. >+class CallbackOperation(CallbackOperationBase): Document that this is used to implement actual WebIDL operations on callback interfaces? >+class CGJSImplInitOperation(CallbackOperationBase): And that this is used to implement constructor bits? >+++ b/dom/bindings/test/TestJSImplGen.webidl >-[Constructor, JSImplementation="@mozilla.org/test-js-impl-interface;1"] >+[Constructor(DOMString arg), JSImplementation="@mozilla.org/test-js-impl-interface;1"] Live large! Add more arguments. Ideally, copy one of the TestCodeGen constructors? Eventually we'll just want to support all the same things here that TestCodeGen can handle... r=me with the nits.

(In reply to Boris Zbarsky (:bz) from comment #27) > This could go in an else branch, right? But that takes one extra line! Changed. > 1) Documentation about why you're slicing off the first two arguments. Fixed. > 2) Assertions about what those arguments are like. Good idea! I asserted on the types and not the names, because the types are GlobalObject and JSContext which are "unique", to make it less fidgety. > Document that this is used to implement actual WebIDL operations on callback > interfaces? > And that this is used to implement constructor bits? Done. > Live large! Add more arguments. I added all of the arguments to all of the constructors to the main interface in TestCodeGen. Two types didn't work, which is expected as they don't work as arguments to interface methods either, so I commented them out with a bug number. Try run: https://tbpl.mozilla.org/?tree=Try&rev=c2beb6d955e9 Just waiting for the tree to reopen.

That didn't take long... https://hg.mozilla.org/integration/mozilla-inbound/rev/6fc3f280d0f6

I added some documentation for this at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Implementing_WebIDL_using_Javascript

ugh sorry