107453 - Rarely used HTML element classes should be combined...

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

There's quite a few HTML element classes in the mozilla code that's not used at
all when loading jrgm's pageload tests, this involves loading 40 top 100 pages,
and should give us a reasonable (but of course not perfect) picture of what code
is commonly used and what's not. When running these tests we don't use any of
these elements:

nsHTMLAppletElement
nsHTMLButtonElement
nsHTMLDelElement
nsHTMLDirectoryElement
nsHTMLEmbedElement
nsHTMLFieldsetElement
nsHTMLInsElement
nsHTMLLabelElement
nsHTMLLegendElement
nsHTMLMenuElement
nsHTMLModElement
nsHTMLObjectElement
nsHTMLOlistElement
nsHTMLOptGroupElement
nsHTMLParamElement
nsHTMLQuoteElement
nsHTMLTableCaptionElement
nsHTMLTableColGroupElement
nsHTMLTableRowElement
nsHTMLTablecolElement
nsHTMLTextareaElement
nsHTMLWbrElement

I've started working on collapsing these unused elements into a few shared
classes that when used will work exactly like the current code at the expence of
a few more vtables on the rarely used element objects.

Patch coming up that collapses nsHTMLTableColGroupElement.cpp and
nsHTMLTableColElement.cpp into nsHTMLTableColElement.cpp, and also
nsTHMLBaseElement.cpp, nsHTMLEmbedElement.cpp, nsHTMLIsIndexElement.cpp,
nsHTMLParamElement.cpp, nsHTMLSpacerElement.cpp and nsHTMLWBRElement.cpp into
nsHTMLSharedLeafElement.cpp. The result of this first step is that we can remove
6 files from the project.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Complete diff for initial pass (mostly leaf elements) excluding nsHTMLSharedLeafElement.cpp

Comment 2

[Johnny Stenback (:jst)]

Attachment: Same as above, excluding deleted files.

Comment 3

[Johnny Stenback (:jst)]

Attachment: Diff from nsHTMLWBRElement to nsHTMLSharedLeafElement

Comment 4

[Chris Waterson]

sr=waterson! Go Johnny go!

Comment 5

[Johnny Stenback (:jst)]

Dbaron, would you review this (second and last patch)?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 55649 [details] [diff] [review]
Same as above, excluding deleted files.

>Index: content/html/content/src/nsHTMLInputElement.cpp
>+  nsresult rv = ((nsGenericHTMLElement *)it)->Init(aNodeInfo);
>+  nsresult rv = ((nsGenericHTMLElement *)it)->Init(mNodeInfo);

Could you use NS_STATIC_CAST, like you did in nsHTMLSelectElement.cpp?

r=dbaron (on the last 2 attachments)

Comment 7

[Johnny Stenback (:jst)]

Yup, will do. Thanks for the review.

Comment 8

[Johnny Stenback (:jst)]

Above patches checked in, more to come...

Comment 9

[Peter Lubczynski]

Is it possible the check-in to this bug is causing the blocker bug 107627?

Comment 10

[Johnny Stenback (:jst)]

Yes, this fix caused bug 107627 (which is now fixed).

Comment 11

[Fabian Guisset]

Fixed, or do we want to keep it open for future similar improvements?

Comment 12

[Johnny Stenback (:jst)]

There's more to do here...

Comment 13

[Johnny Stenback (:jst)]

Pushing to mozilla1.0

Comment 14

[Johnny Stenback (:jst)]

Attachment: Combine rarely used container classes...

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 89166 [details] [diff] [review]
Combine rarely used container classes...

This patch looks fine, and comparing the methods in the old classes to the new
combined class looks correct as well.

r=caillon with the additional caveat of removing (or actually using) the XXX
comment here:
http://lxr.mozilla.org/mozilla/source/content/html/content/src/nsHTMLSharedCont
ainerElem.cpp#344

Comment 16

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 89166 [details] [diff] [review]
Combine rarely used container classes...

>Index: content/html/content/src/Makefile.in
>===================================================================
>-		nsHTMLDelElement.cpp \
>-		nsHTMLDirectoryElement.cpp \
>-		nsHTMLInsElement.cpp \
>-		nsHTMLMenuElement.cpp \
>-		nsHTMLModElement.cpp \
>-		nsHTMLQuoteElement.cpp \
>+		nsHTMLSharedContainerElem.cpp \
>-		nsHTMLTableCaptionElement.cpp \
>Index: content/html/content/src/makefile.win
>===================================================================
>-	.\$(OBJDIR)\nsHTMLDelElement.obj		\
>-	.\$(OBJDIR)\nsHTMLDirectoryElement.obj		\
>-	.\$(OBJDIR)\nsHTMLInsElement.obj		\
>-	.\$(OBJDIR)\nsHTMLMenuElement.obj		\
>-	.\$(OBJDIR)\nsHTMLModElement.obj		\
>-	.\$(OBJDIR)\nsHTMLOListElement.obj		\
>-	.\$(OBJDIR)\nsHTMLQuoteElement.obj		\
>+	.\$(OBJDIR)\nsHTMLSharedContainerElem.obj	\
>-	.\$(OBJDIR)\nsHTMLTableCaptionElement.obj	\

These two lists above don't match (also check Mac project)???

>Index: content/html/content/src/nsGenericHTMLElement.h
>===================================================================
>+inline nsresult
>+NS_NewHTMLOListElement(nsIHTMLContent** aResult, nsINodeInfo *aNodeInfo)
>+{
>+  return NS_NewHTMLSharedContainerElement(aResult, aNodeInfo);
>+}

Is OL really a rarely used element? I wouldn't consider it rare, but...

>Index: content/html/content/src/nsHTMLDelElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLDirectoryElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLInsElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLMenuElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLModElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLOListElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLQuoteElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLTableCaptionElement.cpp
>===================================================================
>Index: content/macbuild/content.xml
>===================================================================
>Index: content/shared/public/nsHTMLAtomList.h
>===================================================================

Comment 17

[Johnny Stenback (:jst)]

Mass-reassigning bugs.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix regression from col/colgroup combining (checked in to trunk, 2004-11-29 21:52 -0800).

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 167388 [details] [diff] [review]
fix regression from col/colgroup combining (checked in to trunk, 2004-11-29 21:52 -0800).

This fixes a regression (perhaps asymptomatic) from attachment 55647 [details] [diff] [review].

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 167388 [details] [diff] [review]
fix regression from col/colgroup combining (checked in to trunk, 2004-11-29 21:52 -0800).

r+sr=bzbarsky

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

File followups if we ever want to do more of this.