Closed
Bug 481652
Opened 16 years ago
Closed 7 years ago
Make DOM interfaces inherit from nsIDOMEventTarget
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
INVALID
People
(Reporter: smaug, Unassigned)
References
Details
Attachments
(1 file, 1 obsolete file)
75.54 KB,
patch
|
Details | Diff | 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.
![]() |
||
Comment 1•16 years ago
|
||
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.
Reporter | ||
Comment 2•16 years ago
|
||
nsDOMClassInfo should handle the 4th parameter of addEventListener.
But the patch does indeed fail in some tests, perhaps because of that reason.
Reporter | ||
Comment 3•16 years ago
|
||
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.
![]() |
||
Comment 4•16 years ago
|
||
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...
Reporter | ||
Comment 5•16 years ago
|
||
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 6•16 years ago
|
||
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, ...).
Reporter | ||
Comment 7•16 years ago
|
||
(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.
Reporter | ||
Comment 8•16 years ago
|
||
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
![]() |
||
Comment 9•16 years ago
|
||
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.
Reporter | ||
Comment 10•16 years ago
|
||
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.
Reporter | ||
Comment 11•16 years ago
|
||
![]() |
||
Comment 12•16 years ago
|
||
> 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.
Reporter | ||
Comment 13•16 years ago
|
||
(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?
![]() |
||
Comment 14•16 years ago
|
||
>> 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.
Reporter | ||
Comment 15•16 years ago
|
||
(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.
Reporter | ||
Comment 16•16 years ago
|
||
(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
Updated•13 years ago
|
Summary: Remove nsIDOMNSEventTarget and make DOM class tree start from nsIDOMEventTarget → Make DOM interfaces inherit from nsIDOMEventTarget
![]() |
||
Comment 18•7 years ago
|
||
There is no more nsIDOMEventTarget, and we are well on the way to having no more DOM interfaces.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Assignee | ||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•