Closed Bug 389797 Opened 12 years ago Closed 12 years ago
[FIX]Make HTML content objects use QI tables
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? ;)
Attachment #274123 - Attachment is obsolete: true
Attachment #274129 - Flags: superreview?(peterv)
Attachment #274129 - Flags: review?(peterv)
Attachment #274129 - Flags: review?(benjamin)
Attachment #274123 - Flags: superreview?(peterv)
Attachment #274123 - Flags: review?(peterv)
Attachment #274123 - Flags: review?(benjamin)
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.
(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...
> 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. ;)
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.
That's still missing the xpcom/base/nsISupportsBase.h comment change; I'll check that in when this lands.
Note to self: mozilla/xpcom/base/nsCycleCollectionParticipant.h is now mozilla/xpcom/glue/nsCycleCollectionParticipant.h
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.
> 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.
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).
Attachment #276580 - Flags: approval1.9? → approval1.9+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.