851178 - Add support for JS implemented WebIDL constructors with arguments

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yes, I agree.  That's basically what I was thinking as well.

we can call the method _init or something like that.

Comment 3

[Andrew McCreight [:mccr8]]

Contacts needs this for mozContact.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Hitting this now in bug 823512 (already linked).

Comment 5

[Andrew McCreight [:mccr8]]

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

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: WIP

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().

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Does this replace nsIDOMGlobalPropertyInitializer?

My __init function is not getting called... Investigating.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: example implementation

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...

Comment 10

[Reuben Morais [:reuben]]

Could we just always call init with window and arguments?

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: WIP (2)

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

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 13

[Andrew McCreight [:mccr8]]

> 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.

Comment 14

[Andrew McCreight [:mccr8]]

> 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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Andrew McCreight [:mccr8]]

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?

Comment 17

[Boris Zbarsky [:bzbarsky]]

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);

Comment 18

[Andrew McCreight [:mccr8]]

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.

Comment 19

[Andrew McCreight [:mccr8]]

Attachment: WIP 3

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

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?

Comment 21

[Andrew McCreight [:mccr8]]

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

Comment 22

[Andrew McCreight [:mccr8]]

Attachment: WIP 4

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.

Comment 23

[Andrew McCreight [:mccr8]]

Does this work for you, jib?

Comment 24

[Boris Zbarsky [:bzbarsky]]

> but that's probably expected.

Yes, absolutely.

Comment 25

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Works! Awesome.

Comment 26

[Andrew McCreight [:mccr8]]

Attachment: v4 + trivial test

Comment 27

[Boris Zbarsky [:bzbarsky]]

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.

Comment 28

[Andrew McCreight [:mccr8]]

(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.

Comment 29

[Andrew McCreight [:mccr8]]

That didn't take long...

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fc3f280d0f6

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6fc3f280d0f6

Comment 31

[Boris Zbarsky [:bzbarsky]]

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

Comment 32

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

ugh sorry