822470 - Do WebIDL codegen for callback interfaces

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Like bug 779048, but callback interfaces; I can reuse a lot of the work bug 779048 did here, I think.

I started poking at this a bit last night. Patches hopefully to come later this week.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Factor out the code that callback functions and callback interfaces will be able to share into a CallbackObject class.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Create a CallbackInterface helper class.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Include headers as needed for callback interfaces.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Expose the concept of "single operation interface" in the WebIDL data model.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Refactor callback codegen so it can be reused for callback interfaces too.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Hook up callback interface codegen.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Use new callback codegen for conversion to and from JS. and EventListener will be updated to use the new codegen once we've fixed the consumers.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Once this lands, I'll do followups for EventListener (which will take a bit of work, I would guess) and NodeFilter (mostly an issue for the Document WebIDL bindings; I'd rather not make them depend on this code or vice versa).

Comment 9

[Boris Zbarsky [:bzbarsky]]

One other note:  Right now we just create a new CallbackInterface object each time we convert JS to C++ here, unlike XPConnect, which looks for an existing XPCWrappedJS.

I would prefer to keep the new behavior and add a way to ask whether two CallbackInterface objects wrap the same JS object for cases like EventListener that actually care about that sort of thing.  Any objections?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Sounds ok, although I assume it will make ELM a tiny bit more complicated.
Will you wrap make wrapper which can deal with EventListenerCallbacks and nsIDOMEventListeners, 
and make nsListenerStruct::mListener to keep such wrapper alive? And the wrapper would have
Equals()?

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 693776 [details] [diff] [review]
part 6.  Hook up callback interface codegen.

Review of attachment 693776 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/CallbackInterface.cpp
@@ +14,5 @@
> +bool
> +CallbackInterface::GetCallableProperty(JSContext* cx, const char* aPropName,
> +				       JS::Value* aCallable)
> +{
> +  if (!JS_GetProperty(cx, mCallback, "doSomething", aCallable)) {

doSomething?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Er, copy-paste fail!  That used to be codegenned, and then I factored it out into a function.  Let me fix that!

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 6 with the obvious mistake fixed

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Will you wrap make wrapper which can deal with EventListenerCallbacks and
> nsIDOMEventListeners, and make nsListenerStruct::mListener to keep such wrapper alive?
> And the wrapper would have Equals()?

That's a good question.  I guess I do need to handle compares between nsIDOMEventListener (coming from XPConnect) and EventListener (coming from WebIDL).  Though if we hook up EventTarget to quickstubs, will we really need that?  I guess Window would still be coming through xpconnect...

This is sounding annoying.  ;)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Well, I was thinking just EventListener comparisons. How would you check in ELM whether some
existing EventListener is the same as the one being set? ELM needs support EventListeners and
nsIDOMEventListeners and would be nice to not have different code paths for WebIDL and XPCOM stuff.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Sure.  For just EventListener, I plan to have a SameObject method or something (need to bikeshed the naming) per comment 9.  The question is whether we'll also need a method to check whether an EventListener and an nsIDOMEventListener wrap the same JS object...

Comment 17

[Andrew McCreight [:mccr8]]

I filed bug 829708 for Splinter not understanding part 1.

Comment 18

[Peter Van der Beken [:peterv]]

Comment on attachment 693772 [details] [diff] [review]
part 2.  Create a CallbackInterface helper class.

Review of attachment 693772 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/CallbackFunction.h
@@ +9,4 @@
>   *
> + * This class implements common functionality like lifetime management,
> + * initialization with the callback object, and setup of the call environment.
> + * Subclasses corresponding to particular callback function types should

s/function/object/ or s/function/interface/?

Comment 19

[Peter Van der Beken [:peterv]]

Comment on attachment 693774 [details] [diff] [review]
part 4.  Expose the concept of "single operation interface" in the WebIDL data model.

Review of attachment 693774 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/parser/tests/test_callback_interface.py
@@ +91,5 @@
> +    for (i, iface) in enumerate(results):
> +      harness.check(iface.isSingleOperationInterface(), i < 4,
> +                    "Interface %s should be a single operation interface" %
> +                    iface.identifier.name)
> +    

Trailing whitespace.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> s/function/object/ or s/function/interface/?
>Trailing whitespace.

Fixed.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated on top of

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 6 with a bug fixed to actually generate code correctly if we have no methods in a callback interface

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 merged to tip

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 merged to tip

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 merged to tip

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4 merged to tip

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5 merged to tip

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 6 merged to tip

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 7 merged to tip

Comment 30

[Boris Zbarsky [:bzbarsky]]

Peter, thanks for the reviews!

   https://hg.mozilla.org/integration/mozilla-inbound/rev/39d1a2977442
   https://hg.mozilla.org/integration/mozilla-inbound/rev/d03fc64bbe76
   https://hg.mozilla.org/integration/mozilla-inbound/rev/e57b8e4373d4
   https://hg.mozilla.org/integration/mozilla-inbound/rev/c8b54141ddd7
   https://hg.mozilla.org/integration/mozilla-inbound/rev/fdad64f0f20d
   https://hg.mozilla.org/integration/mozilla-inbound/rev/02a297c64232
   https://hg.mozilla.org/integration/mozilla-inbound/rev/5517fe5d3fda

Still need to document.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/39d1a2977442
https://hg.mozilla.org/mozilla-central/rev/d03fc64bbe76
https://hg.mozilla.org/mozilla-central/rev/e57b8e4373d4
https://hg.mozilla.org/mozilla-central/rev/c8b54141ddd7
https://hg.mozilla.org/mozilla-central/rev/fdad64f0f20d
https://hg.mozilla.org/mozilla-central/rev/02a297c64232
https://hg.mozilla.org/mozilla-central/rev/5517fe5d3fda

Comment 32

[Boris Zbarsky [:bzbarsky]]

Documented at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Interface_types