Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Description • 11 years ago

Compiling the attached patch with some new webidl initally gave me an error about an undefined RTCConfiguration reference in new RTCPeerConnectionBinding.h. I don't have that error anymore, because when I rebuilt after a clear-screen to get the error back so I could copy+paste it here, the compile somehow got farther.

Seems like it now works thanks to this in RTCPeerConnectionBinding.cpp:
> #include "RTCConfigurationBinding.h"

But there's still no such statement in RTCPeerConnectionBinding.h, even though that header references RTCConfiguration. Andrew says it should have one, so I'm filing this bug anyway.

I'm also attaching the generated RTCPeerConnectionBinding.h file.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 2 • 11 years ago

Patch contains new unused webidl files as well. Only RTCPeerConnection.webidl is in the makefile.

	Andrew McCreight [:mccr8] Assignee
	Comment 4 • 11 years ago

In addition to the include problem, I get this error when I try to build it:
 0:07.52 /Users/amccreight/mz/cent/obj-dbg/dom/bindings/RTCPeerConnectionBinding.cpp:385:10: error: member function 'ToObject' not viable: 'this' argument has type 'const mozilla::dom::RTCConfiguration', but function is not marked const
 0:07.52     if (!configuration.ToObject(cx, mCallback, &argv[0])) {
 0:07.52          ^~~~~~~~~~~~~
 0:07.52 ./RTCConfigurationBinding.h:51:8: note: 'ToObject' declared here
 0:07.52   bool ToObject(JSContext* cx, JSObject* parentObject, JS::Value *vp);
 0:07.52        ^
 0:07.52 1 error generated.

I'm just mentioning that in case that is relevant to this bug.  I'm also on a oldish version of m-c, so that could be a problem.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 11 years ago

Given the attached webidl, RTCPeerConnectionBinding.h should have a forward-declare of RTCConfiguration, I would think, since it doesn't need the actual declaration for the things it's doing with it in the header.  It looks like we're totally failing to do that: we do it for interface types used as arguments, but we should really do it for other things too (callback interfaces, callback functions, dictionaries are the obvious ones, I think).

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 11 years ago

Comment 4 sounds like a bug in our js-implemented codegen for dictionaries: dictionary arguments are const structs, but dictionary return values can't be const because of the ToObject thing.  That said, I wonder why ToObject is not a const method... we should consider fixing that if we can.

	Andrew McCreight [:mccr8] Assignee
	Comment 7 • 11 years ago

I filed bug 863898 for comment 4 and comment 6.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 9 • 11 years ago

I was just about to file this next (I get the same fwiw). Thanks for filing it. :-)

	Andrew McCreight [:mccr8] Assignee
	Comment 10 • 11 years ago

No problem. :)

One interesting thing will be how to test this include bug.  We probably want to compile a dummy .cpp file that only includes a test header referring to various stuff in another file.  Maybe the regular CodeGen header will suffice.

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 11 years ago

Testing this sort of thing is hard.  Best way is to land code that would not build if the bug is reintroduced.  ;)

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 11 years ago

Oh, I have a testcase.  Apply my patch for bug 863898 and try to compile.  ;)  Since we're using Dict in TestJSImplGen.webidl, but it's defined in TestCodeGen.webidl, we hit exactly this bug, at least for the dictionary case.

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 11 years ago

Oh, and for union types too, which we should also forward-declare...

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 11 years ago

So thinking about this some more, the current setup is really ad-hoc.  Why is it currently forward-declaring callbacks it finds in "descriptors"?  Would those actually appear in the binding header somehow?

Seems to me like we need to forward-declare the following: All types (Gecko interfaces, callbacks, dictionaries, callback interfaces, unions) used in callbacks, callback interfaces, and js-implemented interfaces: all of these put a class decl in the header with the function signatures containing the relevant types.  Not sure about types appearing in dictionaries; should check whether we just include headers for those or not.

Right now we seem to forward-declare the following, instead:

1)  Gecko interfaces appearing in dictionaries, callbacks, callback interfaces, and
    js-implemented interfaces.
2)  Unions and callbacks used in callbacks and callback interfaces (but NOT
    js-implemented interfaces).
3)  Callbacks used in dictionaries and non-callback interfaces (which may or may not be
    js-implemented).

So I think we're forward-declaring things we don't need to, and not forward-declaring things we need to, both.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 11 years ago

Oh, and "used" means both arguments and return values, but the geTypesFrom* functions do this right already.

	Andrew McCreight [:mccr8] Assignee
	Comment 17 • 11 years ago

I should probably pull this mess into a CGForwardDeclarations.  Also, the way I hacked around the mutual inclusion problem for the test code is not particularly satisfactory, because it means we're not quite testing the right thing for includes.

	Andrew McCreight [:mccr8] Assignee
	Comment 18 • 11 years ago

Also need to address comments for bug 863964 and rebase over that.

I basically just implemented exactly what bz said in comment 14, then fixed up things that needed to be fixed.  What was missing there is that it seems like a good idea to forward declare everything declared in a header.  Descriptors can be used in PrototypeTraits, PrototypeIDMap, and Wrap.

For the rest of the forward declarations of things in the file, in WebComponents.webidl, the dictionary LifecycleCallbacks refers to LifecycleCreatedCallback, which is defined in the file, after it.  Though come to think of it, rather than just forward declaring everything in the file, I could forward declare any type defined in a dictionary that is defined in the current file (and thus won't be picked up by the header inclusion that dictionaries generate).  I'll look into that and see what else my current approach covers up.

	Andrew McCreight [:mccr8] Assignee
	Comment 21 • 11 years ago

Note that I dropped entirely the forward declaration of XPCWrappedNativeScope, as it doesn't seem to be used anywhere.

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 11 years ago

Comment on attachment 741586 [details] [diff] [review]
rewrite forward declaration generation

So I'm confused about the "workerness" thing.  It's always set to "both" so it always tries to get main-thread descriptors for Gecko interfaces.  But there are Gecko interfaces without main-thread descriptors (e.g. FileReaderSync); if it's not breaking now presumably it's because there is no callback that references that interface...

In practice, I guess this only mattered for mainCallbacks vs workerCallbacks, so I think you probably just want to preserve that distinction.  That will incidentally actually make use of workerness.  r=me with that.

One other note.  isGeckoInterface() is true for callback interfaces (yeah, I know, that's kinda weird).  So I suspect your isCallbackInterface() branch in forwardDeclareForType is dead code.  I'm ok living it in case we ever change how isGeckoInterface behaves, though.

	Andrew McCreight [:mccr8] Assignee
	Comment 23 • 11 years ago

(In reply to Boris Zbarsky (:bz) from comment #22)
> Comment on attachment 741586 [details] [diff] [review]
> rewrite forward declaration generation
> 
> So I'm confused about the "workerness" thing.  It's always set to "both" so
> it always tries to get main-thread descriptors for Gecko interfaces.  But
> there are Gecko interfaces without main-thread descriptors (e.g.
> FileReaderSync); if it's not breaking now presumably it's because there is
> no callback that references that interface...
Oops, I added that for dictionaries, which are split into worker and nonworker, but then late I deleted everything dealing with dictionaries.  Should I add a try catch for the main thread case, too?

> In practice, I guess this only mattered for mainCallbacks vs
> workerCallbacks, so I think you probably just want to preserve that
> distinction.  That will incidentally actually make use of workerness.  r=me
> with that.
Okay, sounds good.

> One other note.  isGeckoInterface() is true for callback interfaces (yeah, I
> know, that's kinda weird).  So I suspect your isCallbackInterface() branch
> in forwardDeclareForType is dead code.  I'm ok living it in case we ever
> change how isGeckoInterface behaves, though.

Okay.

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 11 years ago

> Should I add a try catch for the main thread case, too?

Hmm.  That would work too, yes.

	Andrew McCreight [:mccr8] Assignee
	Comment 25 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/761ecc87c2ca

	Andrew McCreight [:mccr8] Assignee
	Comment 26 • 11 years ago

try run: https://tbpl.mozilla.org/?tree=Try&rev=7ddb2c175154

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/761ecc87c2ca