So the callback stuff has landed now. bz, what additional stuff needs to be implemented for this? At a glance, it looks pretty much the same, but I have no idea really. Maybe I could compare the existing few JS-implemented interfaces to what the callback codegen produces and see what the difference is?

So the way I was thinking of using this was as follows. Add a way to flag an interface as JS-implemented in Bindings.conf. That would do both normal codegen _and_ callback codegen for that interface. At that point we need a class for the normal codegen to call into. Probably the simplest thing to do is to generate yet another class that has a refptr for the CallbackInterface subclass as a member and forwards binding API calls to that object, implements CC appropriately, etc. We'll sort of need to figure out what to do with static methods; maybe we can just fail codegen for them for now if we don't have any of those in JS-implemented stuff. Alternately, we could try to use the CallbackInterface subclass directly as the native of the binding. But that wouldn't let us do interfaces that inherit from another interface (e.g. from EventTarget) and would have various other issues (e.g. the WrapObject stuff that bindings expect doesn't exist on CallbackInterface instances).

I made two copies of the IDL interface, made one a callback, then used the example generator to create a skeleton for the implementation, which I modified to include a refPtr to the callback implementation. The methods are pretty direct, though there is the same issue before about what to do about errors, as the callbacky code generates errors that the outer class doesn't deal with. I think before I recall somebody saying before (when I was looking at doing this with XPIDL instead) that we could propagate the error if possible, and otherwise just ignore the error, asserting in debug builds. I don't know if there's concern about information leakage if the JS implementation throws an object or something. I supposed it doesn't really matter.

> as the callbacky code generates errors that the outer class doesn't deal with. I assume we'd be generating both sets of code from the same IDL interface which will be flagged to generate both callback and non-callback stuff. If so, that same annotation can force all methods/attributes on the interface to be treated as possibly-throwing. That seems to me to be the simplest solution. That would mean that errors would propagate at least in terms of raising a new exception, which I think is the right thing to do. The actual exception object wouldn't propagate in that case.... But this does raise one interesting question. In cases in which an exception is supposed to be thrown by the implementation per spec, we would end up reporting the exception raised by the JS implementation of the interface even if the page catches the exception we throw to it. We may need a way to indicate to callbacks that exceptions should not be reported, and perhaps some way to let the callback callee control what sort of exception is thrown to the original web page...

This adds a new setting jsImplemented to Bindings.conf. When it is set, members will never be infallible.

The callback interface is just a copy of the interface, with "Impl" added to the name. Eventually, this can be autogenerated as part of the bindings when the 'jsImplemented' flag is set.

This patch mangles up the example generator to turn it into a C++ to C++ glue generator. It seems to generate reasonable looking code for the simple getters and setters in the test file. Other than that, the code is mostly bogus, aside from adding a field that owns the callback implementation: nsRefPtr<JSImplImpl> mImpl; The getter/setter code looks like this: int32_t JSImpl::GetFoo(ErrorResult& aRv) const { return mImpl->GetFoo(aRv); } void JSImpl::GetBar(nsString& retval, ErrorResult& aRv) const { mImpl->GetBar(retval, aRv); } void JSImpl::SetBar(const nsAString& arg, ErrorResult& aRv) { mImpl->SetBar(arg, aRv); }

With this, the stack of patches generates a JSImplBinding.cpp file that manages to compile correctly, from the JSImpl.webidl file I attached. It includes the code for the callback class, JSImplImpl, as well as JSImpl and JSImpl's binding. I still need to figure out what we should emit for GetParentObject and WrapObject, emit the CC glue, and do something useful in the constructor. I also need to test it on a wider variety of WebIDL inputs because I'm sure some of the hackier bits of this will break in general.

I have this working now, locally. I still need to do further testing and cleanup. There are some outstanding issues I know about, but maybe they don't need to all be fixed before landing. As part of my cleanup, I'll figure out exactly what those are...

One weird thing is that I had to add in an explicit QI to get the wrapped JS out of it in the constructor. Could do_CreateInstance somehow only be using the QI defined on the XPCOM component? nsCOMPtr<nsISupports> jsImpl0 = do_CreateInstance("${contractId}"); ... nsCOMPtr<nsIXPConnectWrappedJS> jsImpl = do_QueryInterface(jsImpl0); When I had nsIXPConnectWrapperJS instead of nsISupports (and didn't have the QI), then it was returning null.

Having to manually specify that the constructor needs an implicit JSContext is lame, so I'm going to try to get rid of that next. But anyways, this is what a complete implementation looks like in the framework I have set up.

> Could do_CreateInstance somehow only be using the QI defined on the XPCOM component? Possible, yes. Explicit QI to wrappedjs seems fine to me. I still vaguely think we should put the jsImplemented in an extended attr, not the conf file, but I think that about everything. ;) Past that, this is pretty awesome!

(In reply to Boris Zbarsky (:bz) from comment #14) > I still vaguely think we should put the jsImplemented in an extended attr, > not the conf file, but I think that about everything. ;) I personally don't care, I just did it this way because it was easier to figure out. I'll add switching things over to my list of things to fix. I can't imagine it will be that difficult.

Now with methods, automatic implicitJSContext for the constructor, and the contract ID is put in the webidl file rather than the bindings.conf.

Comment on attachment 721370 [details] [diff] [review] part 1: add JSImplementation extended attribute Why are we putting the jsImplemented boolean on DescriptorProvider, not on Descriptor? Seems like it should just live on Descriptor. Also, maybe it should be called isJSImplemented(). >+ def jsImplementation(self): I'd prefer we named this getJSImplementation(). >+ assert len(classId) == 1 How about also asserting that isinstance(classId, list)? r=me with those nits.

> Why are we putting the jsImplemented boolean on DescriptorProvider, not on Descriptor? Seems like it should just live on Descriptor. I did that first, but CGCallback::__init__ takes a DescriptorProvider, not a Descriptor. Is there another way to work around that problem? I'll fix the other things. Looking at the code now, part 2 is quite a bit more copypasta of code I don't understand than I'd like for CGJSImplClass. I guess I'll do some kind of refactoring involving a common base class with CGExampleClass. The other classes don't seem too bad.

> I did that first, but CGCallback::__init__ takes a DescriptorProvider, not a Descriptor. You're using CGCallbackInterface for these things, no? That always has a Descriptor for the interface. Is the problem that you're ending up having to test for this boolean somewhere in CGCallback shared code? If so, where?

Oh, for the name generation? Just make the check whether the provider is in fact a descriptor. I think that would make a lot more sense. Alternately, check whether the IDL object is an interface.

Yes, just for name generation (part 4, in the last chunk for Codegen.py). IIRC, this was actually the only place I needed to hang the boolean off the descriptor at all. In other places, we have an interface available, and could do bool(interface.jsImplementation()) instead, though that's a little gross. I should check that a provider is a descriptor using isinstance?

> I should check that a provider is a descriptor using isinstance? Yep.

Another thought. Descriptor setup should check that the interface is not a C++-implemented interface inheriting from a JS-implemented one. Or vice versa for now. This might be simpler if the IDLInterface itself had a "js implemented" boolean on it.

(In reply to Boris Zbarsky (:bz) from comment #28) > This might be simpler if the IDLInterface itself had a "js implemented" > boolean on it. Okay, if we only need to check a JS implemented predicate when we know we have a descriptor, then I can hang the descriptor off of the IDLInterface instead of the descriptor. IDLInterface already has getJSImplementation(), but I suppose it is more convenient to cache a bool there.

Addressed bz's review comments. Descriptors no longer directly have any information about the JS implementation. Instead, it is stored in two ways only on the interface. First, there is the method |isJSImplemented()| that returns a boolean. Second, there is the method |getJSImplementation()| that returns the string of the contract id, or None if there is none. The former just calls the latter and coerces it to bool. Carrying forward bz's r+.

This patch refactors CGExampleClass into a more generic binding-implementation-generator, CGBindingImplClass. The __init__ for this class takes three methods, cgMethod, cgGetter, cgSetter that are used to codegen things as you would expect. I stick methodDecls on self, because the idea is that the subclass calls init on CGBindingImplClass, then on CGClass, using methodDecls. In addition to changing how it eventually calls init on CGClass, a subclass can define getWrapObjectBody(), getGetParentObjectReturnType() and getGetParentObjectBody(). I confirmed that in the resulting class we generate the same TestExampleInterface-example.{h,cpp} after this patch.

This could probably have been folded into the previous patch, but oh well. The example generator doesn't seem to care, but for JS implemented WebIDL you get an error without this. I'm not sure this is right, but it seems reasonable to me.

This creates stub versions of the various generator classes for JS implemented WebIDL, based on the example generator. This is all basically unchanged from the example version, except I deleted the bodies of the |define| methods, and obviously updated the __init__ to use the JSImpl codegens, rather than the example ones.

This patch is the real meat-and-potatoes of this bug, as it adds the code generators for the constructor, methods, getters and setters. It also adds various goop to the class itself, like field declarations, cycle collector declarations, and various methods needed for the DOM bindings. I factored out name generation for a few things, as it is important that they match up across a few places. As a bonus, I fixed a typo in an unrelated comment.

This part hooks in the code gen at a high level. I implement a new thing for getDescriptors to get all JSImplemented descriptors. We need to generate callback headers even if we don't have any real callback interfaces. This patch adds the invocation of the callback and CGSJImplClass.

Comment on attachment 722562 [details] [diff] [review] part 2: refactor CGExampleClass into more generic base class I don't quite understand the class docstring for CGBindingImplClass. What's a "class binding"? CGBindingImplClass.__init__ needs a docstring explaining what the arguments are. There are still places where we directly append a CGNativeMember to self.methodDecls. Why are those ok? Or are we basically assuming that no one will do that sort of stuff with these objects? I guess for the moment we'll just fail to compile the C++ if someone does, right? If so that's good enough. > if descriptor.wrapperCache: > wrapArgs.append(Argument('bool*', 'aTriedToWrap')) This part should go away. I wonder if we can make the WrapObject be MOZ_OVERRIDE too... Followup bug on that is fine; we'll need to fix ClassMethod to do that. >+ self.methodDecls.insert(0, > ClassMethod("WrapObject", "JSObject*", Please fix the indentation. >+ self.methodDecls.insert(0, > ClassMethod("GetParentObject", Likewise. r=me with the above nits.

Comment on attachment 722563 [details] [diff] [review] part 3: add dependency generation for CGBindingImplClass r=me

Comment on attachment 722564 [details] [diff] [review] part 4: create a stubbed version of the JS impl generator >+ bases=[ClassBase("nsISupports /* Change nativeOwnership in the binding configuration if you don't want this */"), That comment and the one about nsWrapperCache on the next line doesn't make sense for the js impl stuff. There's a lot of copy/paste here, but I'm assuming things will diverge sufficiently that creating a common base class for these getter/setter/method and the example ones wasn't worth it?

> There's a lot of copy/paste here, but I'm assuming things will diverge sufficiently that creating a common base class for these getter/setter/method and the example ones wasn't worth it? Yeah, the next patch changes a bunch of stuff, including the bogus comment (hopefully...). Let me know if you think I should factor things out more. The only thing that really changes is the pile of arguments to the native CG thing, so it didn't seem very worth it.

Comment on attachment 722565 [details] [diff] [review] part 5: fill in the code generators >+ if self.name == 'Constructor': >+ return self.getConstructorImpl() There's nothing that says a method can't be called Constructor, afaict. Probably better to explicitly tell the CGJSImplMethod if it's a constructor... Also, seems like this should happen before you get the callbackArgs and so forth, right? >+ # Add a return if the return type isn't void... pretty hacky. Actually, if Foo is a method returning void in C++, this code: void Foo() { return Bar(); } is perfectly fine. So you can just always prepend the "return". Same thing for getters (which moreover should never have void type anyway). Does that mean you don't nee the retInfo at all? >+ def getConstructorImpl(self): This is assuming a no-argument constructor, right? Probably best to fail codegen for now if the constructor has arguments instead of silently ignoring them. >+ bases=[ClassBase("nsISupports"), >+ ClassBase("nsWrapperCache")], Fix indent, please. >+ return "return %sBinding::Wrap(aCx, aScope, this, aTriedToWrap);" % self.descriptor.name That last argument is gone on trunk. r- because I'd like to see what the changes to deal with constructor properly look like.

Comment on attachment 722566 [details] [diff] [review] part 6: hook up JSiW code gen at high level r=me

Comment on attachment 721376 [details] [diff] [review] add some JS implemented code gen tests Review of attachment 721376 [details] [diff] [review]: ----------------------------------------------------------------- This should probably be landed with the rest to ensure we have at least a minimal level of non-brokenness. Note that it doesn't find everything, as at one point I had a bug where I wasn't actually generating some method bodies.

Comment on attachment 721376 [details] [diff] [review] add some JS implemented code gen tests >+interface TestExampleProxyInterface { Just take this part out. Change the comment in TestCodeGen.webidl to say that things added there should be added here too, please. >+ '__stringifier' : 'Stringify' } Shouldn't need that anymore; we default to it now. r=me with that. We should fix the bugs this found and then try uncommenting more of the test.

(In reply to Boris Zbarsky (:bz) from comment #36) > I don't quite understand the class docstring for CGBindingImplClass. What's > a "class binding"? Yeah, that's pretty awful. How about "Common codegen for generating a C++ implementation of a WebIDL interface"? > CGBindingImplClass.__init__ needs a docstring explaining what the arguments > are. Done. "cgMethod, cgGetter and cgSetter are classes used to codegen methods, getters and setters." > There are still places where we directly append a CGNativeMember to > self.methodDecls. Why are those ok? Or are we basically assuming that no > one will do that sort of stuff with these objects? I guess for the moment > we'll just fail to compile the C++ if someone does, right? If so that's > good enough. Honestly, I have no idea. I don't know what any of that indexed properties, named properties, etc. stuff does. Maybe it would make sense to move that to CGExampleClass, after CGBindingImplClass.__init__, here or in a followup. I could just move anything I don't recognize. It might break the ordering somehow. > > if descriptor.wrapperCache: > > wrapArgs.append(Argument('bool*', 'aTriedToWrap')) > This part should go away. Done. > I wonder if we can make the WrapObject be MOZ_OVERRIDE too... Followup bug > on that is fine; we'll need to fix ClassMethod to do that. I filed bug 851162. > Please fix the indentation. > Likewise. Done. I'll probably upload updated patches in a batch.

> How about "Common codegen for generating a C++ implementation of a WebIDL interface"? Sounds good. > I don't know what any of that indexed properties, named properties, etc. stuff does. It implements <http://dev.w3.org/2006/webapi/WebIDL/#indexed-and-named-properties>, basically. Dealing with that in a followup is fine; just make sure that if you add a: getter long (DOMString); to a JS-implemented interface the result doesn't compile, ok?

(In reply to Boris Zbarsky (:bz) from comment #40) > There's nothing that says a method can't be called Constructor, afaict. > Probably better to explicitly tell the CGJSImplMethod if it's a > constructor... Fixed. I made NamedConstructors say they are constructors. I'm not sure if that's right. In any event, I made those fail an assert because they probably don't work. See below. > Also, seems like this should happen before you get the callbackArgs and so > forth, right? Good point. > is perfectly fine. So you can just always prepend the "return". Same thing > for getters (which moreover should never have void type anyway). Ah, neat! Fixed. > Does that mean you don't nee the retInfo at all? That does appear to be the case. The basic underpinning of the JSWebIDL codegen is that the argument and return types are identical for callback interfaces and normal DOM bindings, so we just mindlessly let things flow through. It seems like this may not be the case for typed arrays, and other things I haven't checked, in which case we may need retInfo, etc. I also use retInfo to assert that a constructor has no args. > >+ def getConstructorImpl(self): > > This is assuming a no-argument constructor, right? Probably best to fail > codegen for now if the constructor has arguments instead of silently > ignoring them. Good point. I fixed this, and confirmed that the assertion triggers appropriately. I also added an assert that the constructor is named "Constructor", which is supposed to cause failures with NamedConstructors, and confirmed that it hits it. I think the way I hacked in implicit JS contexts for constructors will fail for NamedConstructors, so maybe these will break? I filed bug 851178 for supporting constructor arguments. > Fix indent, please. Fixed. > That last argument is gone on trunk. Fixed. > >+interface TestExampleProxyInterface { > Just take this part out. Fixed. > Change the comment in TestCodeGen.webidl to say that things added there > should be added here too, please. Fixed, there and in TestExampleGen.webidl. > >+ '__stringifier' : 'Stringify' } > Shouldn't need that anymore; we default to it now. Fixed. > Dealing with that in a followup is fine; just make sure that if you add a: > getter long (DOMString); > to a JS-implemented interface the result doesn't compile, ok? I added |getter DOMString (unsigned long index);| (your example produced a syntax error, so hopefully what I tried was okay) and it threw an exception because self.body wasn't defined somewhere. I filed bug 851282 for doing something better with these.

Addressed comments, carrying forward bz's r+.

> I made NamedConstructors say they are constructors. Sounds right. > I think the way I hacked in implicit JS contexts for constructors will > fail for NamedConstructors Indeed. Followup bug for when it becomes relevant, if ever. > (your example produced a syntax error, so hopefully what I tried was okay) Ah, I needed "getter long (DOMString name)". In any case, I'd expect both to fail in similar ways.

Addressed review comments. Carrying forward bz's r+.

> Indeed. Followup bug for when it becomes relevant, if ever. Filed bug 851287. And yes, your corrected example does fail, in the same way.

Comment on attachment 725094 [details] [diff] [review] part 5: fill in the code generators r=me, though it might be better to make these: + assert self.name == 'Constructor' + assert len(self.signature[1]) == 0 into actual exceptions that have the text of the comments in them as the exception text.

I should add some basic documentation on this when I land it, to the DOM bindings page, talking about how we only support Constructor right now, what the JSImplementation thing is, and how the JS implementation only has to implement the QI for nsISupports, and give a pointer to the page on implementing an XPCOM thing using JS.

I switched the assertions to TypeError exceptions locally. I don't have any sense of how breakage prone this will be (probably not very...), so I'm going to do a try run and then land this over the weekend assuming all goes well. https://bugzilla.mozilla.org/show_bug.cgi?id=827486

I would expect this to be very breakage-resistant, since almost all the code you're changing is not part of the build right now. ;)

Shows what you know, my try push is entirely broken ;) https://tbpl.mozilla.org/?tree=Try&rev=5ccaed024e42 (that's what I meant to paste in above) I guess I'll just go ahead and land it this weekend. I guess the main problem would be bustage on gcc or visual studio for my test build thing. The code is pretty simple so it should be okay.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/db5c280d1907 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c41dd4249fe9 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/130895fa32f4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/56d26727c900 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9b5c9e5beba9 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c832fed8e85b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f408dd29944c

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f5a19977d51e - the Windows crashes in mozilla::dom::InterfaceHasInstance probably mean that you need to file a bug about it requiring a clobber, and do the /CLOBBER and clobberer dance to reland it.

Weird. My patch shouldn't have resulted in any differences in outputted code. I guess I'll try to figure out what it did change, then file a bug for khuey to fix. ;) Try run to confirm that I didn't just somehow break something on Windows: https://tbpl.mozilla.org/?tree=Try&rev=44ababa5a6a1

It looks like ms2ger hit the same clobber problem in bug 851908.

Repushed with a clobber pointing to 851908 in the final changeset. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/218356c595fc remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3ae4b90221ee remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/976579f8e55f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3117a41c48ea remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4919ef4c63c3 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f3a58b8ac8b1 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2ea5f267af77

Backed out again in https://hg.mozilla.org/integration/mozilla-inbound/rev/d75e34da1a9f - two Windows PGO builds on your push and one later-on Windows debug build died in ipc\ipdl with a not-very-forthcoming make.py[7]: Entering directory 'e:\builds\moz2_slave\m-in-w32-pgo-00000000000000000\build\obj-firefox\ipc\ipdl' Traceback (most recent call last): File "e:/builds/moz2_slave/m-in-w32-pgo-00000000000000000/build/build/pymake/make.py", line 21, in <module> pymake.process.ParallelContext.spin() File "e:\builds\moz2_slave\m-in-w32-pgo-00000000000000000\build\build\pymake\pymake\process.py", line 526, in spin c.run() File "e:\builds\moz2_slave\m-in-w32-pgo-00000000000000000\build\build\pymake\pymake\process.py", line 456, in run cb(*args, **kwargs) File "e:\builds\moz2_slave\m-in-w32-pgo-00000000000000000\build\build\pymake\pymake\data.py", line 744, in doresolve r.resolvedeps(False, self.resolvecb) File "e:\builds\moz2_slave\m-in-w32-pgo-00000000000000000\build\build\pymake\pymake\data.py", line 815, in resolvedeps self._resolvedepsparallel() File "e:\builds\moz2_slave\m-in-w32-pgo-00000000000000000\build\build\pymake\pymake\data.py", line 879, in _resolvedepsparallel self.makefile.context.defer(self._startdepparallel, d) File "e:\builds\moz2_slave\m-in-w32-pgo-00000000000000000\build\build\pymake\pymake\process.py", line 460, in defer self.pending.append((cb, args, kwargs)) MemoryError

If these patches affect anything in the ipc/ipdl build, we have problems beyond belief, imo. I claim that was a random coincindence....

khuey thinks that the clobber problem (bug 851908) was fixed by bug 462463, so I bravely re-repushed this sans-clobber. Hopefully the PGO problems were also random... remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e4a689014130 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/32851c16ed45 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8d31fbd56129 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e85d11e93d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9abdb274d5c8 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/07c51bdbd801 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2490b7e423d6

khuey backed this out again in 20152b50f68a. The current theory is that bug 462463 somehow caused this to OOM pymake. We triggered a PGO build prior to backing this bug out, but after backing out 462463 to test the theory.

It looks like bug 462463 was causing the PGO problems: PGO builds with the patches here but with bug 462463 backed out succeeded (all three windows PGO builds had all green tests even!). Re-re-re-landed, with a clobber in the last patch, now that bug 462463 is backed out. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/649be71ae43b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0201a604e8e6 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4958713cfe14 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2f6ae75fb124 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe1bc48dd10 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8212a0b3bbe7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d047a7ed2871

I triggered a PGO build on my push just in case...

https://hg.mozilla.org/mozilla-central/rev/649be71ae43b https://hg.mozilla.org/mozilla-central/rev/0201a604e8e6 https://hg.mozilla.org/mozilla-central/rev/4958713cfe14 https://hg.mozilla.org/mozilla-central/rev/2f6ae75fb124 https://hg.mozilla.org/mozilla-central/rev/7fe1bc48dd10 https://hg.mozilla.org/mozilla-central/rev/8212a0b3bbe7 https://hg.mozilla.org/mozilla-central/rev/d047a7ed2871

I added some basic documentation of how to implement a WebIDL interface in JS. https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings