Closed Bug 389797 Opened 12 years ago Closed 12 years ago

[FIX]Make HTML content objects use QI tables

Categories

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

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9beta1

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Fix (obsolete) — Splinter Review
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 - Flags: superreview?(peterv)
Attachment #274123 - Flags: review?(peterv)
Attachment #274123 - Flags: review?(benjamin)
Attached patch Fix a pair of typos (obsolete) — Splinter Review
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.  ;)
Attachment #274129 - Flags: review?(benjamin) → review+
Attached patch Now with test (obsolete) — Splinter Review
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.
Attachment #274129 - Attachment is obsolete: true
Attachment #275150 - Flags: superreview?(jst)
Attachment #275150 - Flags: review?(jst)
Attachment #274129 - Flags: superreview?(peterv)
Attachment #274129 - Flags: review?(peterv)
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.
Attachment #275150 - Flags: superreview?(jst)
Attachment #275150 - Flags: superreview+
Attachment #275150 - Flags: review?(jst)
Attachment #275150 - Flags: review+
> 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 #275150 - Attachment is obsolete: true
Attachment #276580 - Flags: approval1.9?
Checked in.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.