Add support for JS implemented WebIDL constructors with arguments

RESOLVED FIXED in mozilla23

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 3 bugs, {dev-doc-complete})

unspecified
mozilla23
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Updated

5 years ago
Blocks: 823512
(Assignee)

Comment 1

5 years ago
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.
Blocks: 850430
(Assignee)

Comment 3

5 years ago
Contacts needs this for mozContact.
Hitting this now in bug 823512 (already linked).
(Assignee)

Comment 5

5 years ago
I'll work on this after I finish bug 851639.
Assignee: nobody → continuation
Great! This is the last real blocker to something in bug 823512 we can turn on.
(Assignee)

Comment 7

5 years ago
Created attachment 744943 [details] [diff] [review]
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().
Does this replace nsIDOMGlobalPropertyInitializer?

My __init function is not getting called... Investigating.
(Assignee)

Comment 9

5 years ago
Created attachment 745373 [details] [diff] [review]
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...
Attachment #745373 - Flags: checkin-
Could we just always call init with window and arguments?
Created attachment 745946 [details] [diff] [review]
WIP (2)

Unbitrotted and changed with bz's help to work with cx argument which got added over the weekend.
Attachment #744943 - Attachment is obsolete: true
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.
(Assignee)

Comment 13

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

Comment 14

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

Comment 16

5 years ago
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?
(Assignee)

Updated

5 years ago
Blocks: 869112
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);
(Assignee)

Updated

5 years ago
Blocks: 869268
(Assignee)

Comment 18

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

Comment 19

5 years ago
Created attachment 746191 [details] [diff] [review]
WIP 3

I cleaned up the code a bit, but nothing substantial.
Attachment #745946 - Attachment is obsolete: true
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?
(Assignee)

Comment 21

5 years ago
Ah, good point!  So I should be able to just copy pasta some code into Constructor then ignore _constructor.
(Assignee)

Comment 22

5 years ago
Created attachment 746530 [details] [diff] [review]
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.
Attachment #746191 - Attachment is obsolete: true
(Assignee)

Comment 23

5 years ago
Does this work for you, jib?
Flags: needinfo?(jib)
> but that's probably expected.

Yes, absolutely.
Works! Awesome.
Flags: needinfo?(jib)
(Assignee)

Comment 26

5 years ago
Created attachment 746598 [details] [diff] [review]
v4 + trivial test
Attachment #746530 - Attachment is obsolete: true
Attachment #746598 - Flags: review?(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.
Attachment #746598 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 28

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

Comment 29

5 years ago
That didn't take long...

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fc3f280d0f6
https://hg.mozilla.org/mozilla-central/rev/6fc3f280d0f6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
I added some documentation for this at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Implementing_WebIDL_using_Javascript
Keywords: dev-doc-complete
Whiteboard: gain control
ugh sorry
Whiteboard: gain control
You need to log in before you can comment on or make changes to this bug.