Closed Bug 481652 Opened 14 years ago Closed 4 years ago

Make DOM interfaces inherit from nsIDOMEventTarget

Categories

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

x86
All
defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: smaug, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch WIP, not fully tested (obsolete) — Splinter Review
Just an idea to remove one pretty commonly used tearoff class nsDOMEventRTTearoff
and to simplify adding/removing event listeners.
Currently
nsPIDOMEventTarget->nsINode->nsIContent->nsGeneriElement/nsGenericDOMDataNode
                                               |
                                          nsDOMEventRTTearoff
                                          nsIDOMEventTarget
                                          nsIDOMNSEventTarget
                                          nsIDOM3EventTarget
The patch changes that to
nsIDOMEventTarget->nsIDOM3EventTarget->nsPIDOMEventTarget->nsINode->nsIContent->nsEventTargetContent->nsGeneriElement/nsGenericDOMDataNode

nsEventTargetContent is just a small helper class to implement most of the
event handling related methods.
nsIDOMNSEventTarget is scriptable for a reason.  It's so chrome can register for untrusted events if it wants to.  If we don't have tests for this (which this patch would fail), we should.
nsDOMClassInfo should handle the 4th parameter of addEventListener.
But the patch does indeed fail in some tests, perhaps because of that reason.
The failures were because of nsDOMAttribute QI didn't return anything
for nsISupports.

We do have tests for 4th parameter of .addEventListener.
Most of them are in a quite strange place, in XHR's event handling test.

Running rest of mochitest. chrome and browser-chrome passed.
Oh, hmm.  Yeah, you're right.  Classinfo does handle it for now.  But we wanted to move to annotated IDL here.  And we can't annotate nsIDOMEventListener, since that would break binary compat on a frozen interface...
We can't change nsIDOMEventListener and currently our idl doesn't allow
methods with same names in the inheritance tree.
If we really need nsIDOMNSEventTarget for something, it could stay as a
tearoff.
Attachment #365660 - Attachment is obsolete: true
Comment on attachment 365685 [details] [diff] [review]
passes mochitest, chrome and browser-chrome

>-  NS_NODE_INTERFACE_TABLE7(nsDOMAttribute, nsIDOMAttr, nsIAttribute, nsINode,
>-                           nsIDOMNode, nsIDOM3Node, nsIDOM3Attr,
>-                           nsPIDOMEventTarget)
>+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIAttribute)

This is not at all equivalent, you're removing the thisptr offset table for attributes. Please don't switch to the generic macros unless you have a good reason (note that there's a small error in the existing code: nsINode shouldn't be in the list, it's already added by the NS_NODE_INTERFACE_TABLE7 macro itself).
Might also be worth adding nsIDOMEventTarget and nsIDOM3EventTarget to the offset tables of the various nodes (NS_HTML_CONTENT_INTERFACE_TABLE_AMBIGUOUS_BEGIN, NS_DOCUMENT_INTERFACE_TABLE_BEGIN, ...).
(In reply to comment #6)
> (From update of attachment 365685 [details] [diff] [review])
> >-  NS_NODE_INTERFACE_TABLE7(nsDOMAttribute, nsIDOMAttr, nsIAttribute, 
Ah, my mistake. I was thinking NS_INTERFACE_TABLE7 here.
Btw, I think we can remove also nsIDOM3EventTarget, nsIDOM3DocumentEvent and
nsIDOMEventGroup. Those are based on some very old DOM 3 Events draft and
not properly implemented nor exposed to web (one has to use .QueryInterface).
We support only default and system event group and our C++ code which
occasionally uses system event group could use nsPIDOMEventTarget.
One problem is this http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/events/Makefile.in?mark=49-49,54-56#49

This all is for some other bug though.

But I'd like to ask, does anyone object removing nsIDOMNSEventTarget and
making nsIDOMEventTarget the base class for nsINodes?
I'm not sure what comment 4 tries to say about .addEventListener
Depends on: 481863
Ugh.  More unfrozen interfaces in SDK_XPIDLSRCS.  Can you please file a separate bug on that?  I think we should just move it out of there...

Comment 4 says that we want to get rid of the classinfo goop and implement the 4-argument version of addEventListener via an IDL method with annotations.  See bug 459452 and bug 428229.  As written, it looks like this patch would break that.
How does bug 459452 handle the case where the same method name is used
in two interfaces (which both are implemented by same object),
but only one of the methods has some of its parameters optional.
(In reply to comment #9)
> Ugh.  More unfrozen interfaces in SDK_XPIDLSRCS.
Bug 275220.
> How does bug 459452 handle the case where the same method name is used

The same way IDL already handles it; we already have this situation for other cases (e.g. same names but different signatures).  One of the interfaces wins when called from JS.  Which one presumably depends on the xpconnect guts details.  We'd want the 4-arg versioon to always win for calls from JS.
(In reply to comment #12)
> The same way IDL already handles it;
xpidl gives me an error if the same name is used.

> we already have this situation for other
> cases (e.g. same names but different signatures).
With which interfaces?
>> The same way IDL already handles it;
>xpidl gives me an error if the same name is used

Not if it's used on different interfaces implemented by the same object.

> With which interfaces?

nsISupportsCString has a |data| attribute of type ACString.  We have lots of other interfaces that have attributes called |data| of various types (integers, PRTime, ACString, DOMString, AString, nsISupports).  Any time two such interfaces are implemented on the same object, a call from JS will pick a GetData function to call and call it.
(In reply to comment #14)
> Not if it's used on different interfaces implemented by the same object.
Sure if one doesn't inherit the other.

I guess there isn't any reasonable way to implement this all the way I'd want to.
(In reply to comment #14)
>  Any time two such
> interfaces are implemented on the same object, a call from JS will pick a
> GetData function to call and call it.
But which GetData is there are several GetDatas with different parameters.

I guess I could keep nsIDOMNSEventTarget as it is, implemented as a tearoff,
and just make nsIDOMEventTarget the base object of nsPIDOMEventTarget.
Olli, what do you want to do with this bug? It feels like it's mostly fixed by bug 658714
Summary: Remove nsIDOMNSEventTarget and make DOM class tree start from nsIDOMEventTarget → Make DOM interfaces inherit from nsIDOMEventTarget
Depends on: 1429903
There is no more nsIDOMEventTarget, and we are well on the way to having no more DOM interfaces.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INVALID
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.