708061 - Align our node class hierarchy with the spec prototype hierarchy

Status: RESOLVED WORKSFORME

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

Description

[Boris Zbarsky [:bzbarsky]]

This is basically a prereq for bug 622300.

The way quickstubs are implemented right now is that the quickstub functions are per-interface.  That means that the methods have to QI to the right interface and such.  Some of the custom quickstubs cast to particular concrete classes in situations where we know that all implementations of the interface in question are of that class, but this has caused trouble in the past in various ways.

I would prefer, and we've all discussed this in the past, to move to a setup where the quickstub functions (or new dom binding functions, same thing) are per-prototype-object.  For interfaces that are mix-ins or whatnot that means there may be lots of duplicated versions of the function by default, but I think that's ok.  And for cases we care about we could create custom things there too.  In any case, I hope that partial interfaces will be used more often than mixins.

In any case, the goal is the following: For any given DOM prototype, there is a single concrete C++ class such that all functions exposed on that prototype are exposed on that class.

What that class should be is an interesting question.  Some are pretty clear.  For example, all methods of Node.prototype should have corresponding versions on nsINode.  All methods of Element.prototype should have corresponding versions on Element or nsGenericElement.  All methods of HTMLElement.prototype should have corresponding versions on nsGenericHTMLElement.  All methods of Document.prototype should have corresponding versions on nsDocument or nsIDocument.  All that is pretty straightforward.

The more interesting parts are things like .style.  This lives on an interface implemented by both nsXULElement and nsGenericHTMLElement (and also the SVG stuff).  Right now, the custom quickstub for the xul+html version of .style uses a shared superclass of the two, but then it has to watch out for other instances of that superclass which do NOT have that same .style on them.  In the new world, we would have two separate .style getters: one on HTMLElement.prototype and one on XULElement.prototype.  The former would unwrap to nsGenericHTMLElement and the latter would unwrap to nsXULElement.  The actual C++ implementation can of course still live on some superclass of the two.

In any case, this bug is about whatever changes to our core DOM are needed to make this possible.  That means exposing new methods on things like nsINode, nsGenericElement, nsDocument, etc.  It means finding whatever places the DOM prototype setup doesn't match our class hierarchy and fixing them.  I would hope there are not many of those.

Comment 1

[Bobby Holley (:bholley)]

We've been talking about this on IRC. It sounds like the current plan is to have a list of implemented interfaces in the most-derived JSClass. We then check the list during unwrapping. roc has a clever trick where we enumerate the interfaces from most-abstract downward, meaning that we don't even have to crawl the list, since the interface we're looking for is always at a constant offset.

Over email, Ms2ger identified the issues he found. We discussed them over IRC, and I think we're ok:

> Core DOM: DocumentFragment, as noted. (Bug 563659)

Not a problem, since we can just avoid including that interface in the list.
 
> HTML: nsHTMLSharedElement / nsHTMLSharedObjectElement / nsHTMLSharedListElement

Not a problem, we just need to have different JSClasses for the different kinds of nsHTMLSharedElement.
 
> SVG: SVGTextContentElement / SVGTextPositioningElement: <http://mxr.mozilla.org> /mozilla-central/source/content/svg/content/src/nsSVGTextElement.cpp#63>

A diamond-inheritance workaround. I don't think this affects us.

So it looks like we don't have to move anything around to be ready for the new DOM bindings. Resolving WFM for lack of a better resolution indicator.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

What about the fact that EventTarget isn't on the prototype chain anywhere? EventTarget should be the base interface for Node.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hrm.  In the C++ this is actually OK: nsINode inherits from nsIDOMEventTarget.

But we probably need to change nsIDOMNode to inherit from nsIDOMEventTarget to make the prototype setup work correctly.  Which means we should probably switch the parent class of nsINode from nsIDOMEventTarget to some internal class that has the event target stuff on it or something...  This part will actually take some serious surgery.  :(

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think I would prefer to inherit nsIDOMEventTarget twice. IIRC we already do that for some other classes (window?).

Or we just wait for the new DOM bindings where we hopefully won't have to have a separate idl-interface chain separate from the class hierarchy.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I think I would prefer to inherit nsIDOMEventTarget twice.

I'd be fine with that.

This bug is somewhat a prereq for the new DOM bindings, no?

Comment 6

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

I don't recall any case when nsIDOMEventTarget is inherited twice.

I've tried to make nsIDOMEventTarget to be the topmost class (except nsISupports),
nsGlobalWindow needs still fixing.
It would simplify some code if we could rely on nsIDOMEventTarget to be the base class.