Closed Bug 822470 Opened 12 years ago Closed 12 years ago

Do WebIDL codegen for callback interfaces

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(7 files, 10 obsolete files)

(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
peterv
: review+
Details | Diff | Splinter Review
(deleted), patch
peterv
: review+
Details | Diff | Splinter Review
(deleted), 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 #693772 - Flags: review?(peterv)
Attachment #693773 - Flags: review?(peterv)
Attached patch part 6. Hook up callback interface codegen. (obsolete) (deleted) — Splinter Review
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!
Attached patch Part 6 with the obvious mistake fixed (obsolete) (deleted) — Splinter Review
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
Attached patch Updated on top of (obsolete) (deleted) — Splinter Review
Attachment #702509 - Flags: review?(peterv)
Attachment #693999 - Attachment is obsolete: true
Attachment #693999 - Flags: review?(peterv)
Attached patch Part 1 merged to tip (deleted) — Splinter Review
Attachment #693771 - Attachment is obsolete: true
Attached patch Part 2 merged to tip (deleted) — Splinter Review
Attachment #693772 - Attachment is obsolete: true
Attached patch Part 3 merged to tip (deleted) — Splinter Review
Attachment #693773 - Attachment is obsolete: true
Attached patch Part 4 merged to tip (deleted) — Splinter Review
Attachment #693774 - Attachment is obsolete: true
Attached patch Part 5 merged to tip (deleted) — Splinter Review
Attachment #704006 - Flags: review?(peterv)
Attachment #693775 - Attachment is obsolete: true
Attachment #693775 - Flags: review?(peterv)
Attached patch Part 6 merged to tip (deleted) — Splinter Review
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)
Attached patch Part 7 merged to tip (deleted) — Splinter Review
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: