Do WebIDL codegen for callback interfaces

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
6 years ago
a month ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla21
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 10 obsolete attachments)

21.66 KB, patch
Details | Diff | Splinter Review
3.92 KB, patch
Details | Diff | Splinter Review
10.21 KB, patch
Details | Diff | Splinter Review
3.58 KB, patch
Details | Diff | Splinter Review
16.26 KB, patch
peterv
: review+
Details | Diff | Splinter Review
19.34 KB, patch
peterv
: review+
Details | Diff | Splinter Review
19.31 KB, patch
peterv
: review+
Details | Diff | Splinter Review
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.
Blocks: 822213
Attachment #693776 - Flags: review?(peterv)
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).
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?
Flags: needinfo?(bugs)
Flags: needinfo?(peterv)
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()?
Flags: needinfo?(bugs)
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?
Er, copy-paste fail!  That used to be codegenned, and then I factored it out into a function.  Let me fix that!
Attachment #693999 - Flags: review?(peterv)
Attachment #693776 - Attachment is obsolete: true
Attachment #693776 - Flags: review?(peterv)
> 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.  ;)
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.
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...
Whiteboard: [need review]
I filed bug 829708 for Splinter not understanding part 1.
Attachment #693771 - Flags: review?(peterv) → review+
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/?
Attachment #693772 - Flags: review?(peterv) → review+
Flags: needinfo?(peterv)
Attachment #693773 - Flags: review?(peterv) → review+
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.
Attachment #693774 - Flags: review?(peterv) → review+
> s/function/object/ or s/function/interface/?
>Trailing whitespace.

Fixed.
Blocks: 830099
Blocks: 830992
Posted patch Updated on top of (obsolete) — Splinter Review
Attachment #702509 - Flags: review?(peterv)
Attachment #693999 - Attachment is obsolete: true
Attachment #693999 - Flags: review?(peterv)
Attachment #693771 - Attachment is obsolete: true
Attachment #693772 - Attachment is obsolete: true
Attachment #693773 - Attachment is obsolete: true
Attachment #693774 - Attachment is obsolete: true
Attachment #704006 - Flags: review?(peterv)
Attachment #693775 - Attachment is obsolete: true
Attachment #693775 - Flags: review?(peterv)
Attachment #704007 - Flags: review?(peterv)
Attachment #702632 - Attachment is obsolete: true
Attachment #702632 - Flags: review?(peterv)
Attachment #702509 - Attachment is obsolete: true
Attachment #702509 - Flags: review?(peterv)
Attachment #704008 - Flags: review?(peterv)
Attachment #693777 - Attachment is obsolete: true
Attachment #693777 - Flags: review?(peterv)
Attachment #704006 - Flags: review?(peterv) → review+
Keywords: dev-doc-needed
Attachment #704007 - Flags: review?(peterv) → review+
Attachment #704008 - Flags: review?(peterv) → review+
Blocks: 835643
Blocks: 862627
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.