389797 - [FIX]Make HTML content objects use QI tables

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Patch is attached.  Summary of changes:

1)  Some restructuring of macros in nsISupportsImpl.h to allow using a QI table
    together with other things easily.
2)  Some macros to glue together a QI table (starting the QI impl) and a QI map
    (ending the QI impl).
3)  Some additions to the cycle collection macros.
4)  Updated QI impls for all sorts of stuff.  I tested rolling
    DOMQueryInterface into the QI tables for all the classes, and that
    actually gave larger codesize, so I left it as-is.  Benjamin, can you
    double-check that the way I use nsIDOMHTMLElement here is ok?
5)  Removal of some explicit nsSupportsWeakReference inheritance and some
    explicit QIs to nsISupportsWeakReference in HTML classes, since this is
    now supported via a tearoff on nsGenericElement.

Patch reduces Z by a few KB.  Not really all that much, in the end.  It does basically move a few more KB of code into data, though.

I need to figure out a good way to test that I got all the classinfo names still right and didn't drop any interfaces in the various QI methods...  Not sure how to do that, esp. for the non-scriptable stuff.  :(  Maybe I can just rely on code review to catch issues?  ;)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix a pair of typos

Comment 2

[Mats Palmgren (inactive)]

A few nits I spotted while glancing through the patch...

> xpcom/glue/nsISupportsImpl.h
> #define NS_INTERFACE_TABLE_HEAD(_class)             
>  ...
>  nsresult rv = NS_ERROR_FAILURE;;

Please remove one semicolon while you're here.

Also, the documentation for nsISupports::QueryInterface says that
NS_ERROR_INVALID_POINTER is returned if aInstancePtr is null.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/base/nsISupportsBase.h&rev=3.9&root=/cvsroot&mark=83#66

> content/html/document/src/nsImageDocument.cpp

The stuff for ImageListener should be removed completely - there is
no need to disambiguate between multiple nsISupports superclasses here.

Comment 3

[Mats Palmgren (inactive)]

(In reply to comment #2)
> Also, the documentation for nsISupports::QueryInterface says that
> NS_ERROR_INVALID_POINTER is returned if aInstancePtr is null.

What I meant was that we should update the documentation to say
that aInstancePtr must be valid and that we don't null-check it...

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Also, the documentation for nsISupports::QueryInterface says

I can agree with fixing the docs, sure.

> The stuff for ImageListener should be removed completely

Sure.  I was sorta in mechanical-change mode by then.  ;)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Now with test

Adds test for all the scriptable stuff, which caught me losing nsIInterfaceRequestor on embed/applet/object.  This patch passes the test and also addresses Mats' comments.

Comment 6

[Boris Zbarsky [:bzbarsky]]

That's still missing the xpcom/base/nsISupportsBase.h comment change; I'll check that in when this lands.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Note to self: mozilla/xpcom/base/nsCycleCollectionParticipant.h is now mozilla/xpcom/glue/nsCycleCollectionParticipant.h

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 275150 [details] [diff] [review]
Now with test

The changes in nsGenericHTMLElement::FindForm() look unrelated to this change.

Why is the support for nsSupportsWeakReference is dropped from nsHTMLFormElement and nsHTMLObjectElement

+NS_HTML_CONTENT_INTERFACE_TABLE_HEAD(nsHTMLFrameElement,
+                                      nsGenericHTMLFrameElement)

Off-by-one indentation. Same in nsHTMLInputElement.cpp, nsHTMLLableElement.cpp, nsHTMLLegendElement.cpp, nsHTMLOptGroupElement.cpp, nsHTMLOptionElement.cpp, nsHTMLParagraphElement.cpp, nsHTMLTableRowElement.cpp, nsHTMLTextArea.cpp

- In nsImageDocument.cpp:

+// XXXbz shouldn't this participate in cycle collection?  It's got
+// mImageContent!

Yeah, probably should. Wanna file a bug on that? Same for nsPluginDocument

The nsPluginStreamListener::OnStartRequest() changes look unrelated.

The nsObjectLoadingContent changes look unrelated as well.

r+sr=jst with that looked at.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> The changes in nsGenericHTMLElement::FindForm() look unrelated to this change.

Er... yes.  Too many patches.  :(

> Why is the support for nsSupportsWeakReference is dropped 

All elements implement nsISupportsWeakReference via a tearoff in nsGenericElement now.  It didn't seem worth it to leave the explicit support in these subclasses (with its vtable pointer, etc).  I can put it back if you think the performance benefits are worth it, but I doubt they are.

> Off-by-one indentation.

Doh.  Must be left over from when I renamed that macro... will fix.

> Yeah, probably should. Wanna file a bug on that?

Will do.

> The nsPluginStreamListener::OnStartRequest() changes look unrelated.

Indeed.  Will remove.

> The nsObjectLoadingContent changes look unrelated as well.

The nsSupportsWeakReference part is related; the other part I'll remove.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to all comments, inclusing nsISupportsBase comment fix

This is a change to make HTML DOM objects use QI tables.  This reduces codesize somewhat, and with any luck might also improve performance a tiny bit.  Risk is pretty small: the only real risk is that I dropped an interface somewhere, and I checked them over pretty carefully (including a test for all the scriptable ones).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Checked in.