276959 - get rid of nsIHTMLContent

Status: RESOLVED FIXED

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

Description

[Brian Ryner (not reading)]

It's not really necessary for us to have nsIHTMLContent.  The interface isn't
used outside of gklayout, so the implementations can just move onto
nsGenericHTMLElement, and everything except GetAttributeMappingFunction can
become non-virtual.

(I'd like to do the same with nsIXMLContent and nsIStyledContent)

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Pretty much what I described above.  The relatively few callers that need to
deal with the nsIHTMLContent methods now use nsGenericHTMLElement directly. 
Rather than put an IID on nsGenericHTMLElement, I added a method that does an
IsContentOfType check and cast, which should be more efficient than QI because
it doesn't addref.

I went ahead and changed GetAttributeMappingFunction to just return the
function pointer, since this method never fails.

I replaced some code in nsHTMLSelectElement with what I belive to be much
simpler code that does the same thing.	Same goes for nsIsIndexFrame and
nsTextControlFrame.  Hopefully there's not some subtle behavior here that I'm
missing...

Comment 2

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

also note bug 236476

Comment 3

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

Oh, and like so much else MaybeTriggerAutoLink could probably be replaced by a
proper fix for bug 239152

Comment 4

[Peter Van der Beken [:peterv]]

(In reply to comment #0)
> (I'd like to do the same with nsIXMLContent and nsIStyledContent)

Another one is nsIXULContent. It's used in one place outside content
(http://lxr.mozilla.org/seamonkey/source/widget/src/mac/nsDragService.cpp#144),
and that can easily be switched to nsIContent and IsContentOfType(eXUL).

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 170221 [details] [diff] [review]
patch

- In nsHTMLSharedElement::GetAttributeMappingFunction()

-  else if (mNodeInfo->Equals(nsHTMLAtoms::spacer)) {
-    aMapRuleFunc = &SpacerMapAttributesIntoRule;
+  if (mNodeInfo->Equals(nsHTMLAtoms::spacer)) {
+    return &SpacerMapAttributesIntoRule;
   }
   else if (mNodeInfo->Equals(nsHTMLAtoms::dir) ||
	    mNodeInfo->Equals(nsHTMLAtoms::menu)) {

Get rid of this else-after-return as well.

r+sr=jst with that change. Looks good.

Comment 6

[Brian Ryner (not reading)]

checked in.

Comment 7

[[e-mail]]

Attachment: Correct fix for QI error

Changing the type of mCurrentForm saves code.

Either of you feel free to r+sr this if you like it that much ;-)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Doesn't that cause an extra QI on every SetForm call instead of just one QI up
front?

Comment 9

[Brian Ryner (not reading)]

Comment on attachment 171165 [details] [diff] [review]
Correct fix for QI error

Yeah, that's fine too.	Yet another idea would be to declare nsHTMLFormElement
outside of the .cpp file, so this code can see that it inherits
nsIDOMHTMLFormElement, then either make NS_NewHTMLFormElement return that
pointer type or cast the result to it.	Probably doesn't matter that much.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 171165 [details] [diff] [review]
Correct fix for QI error

sr=jst

Comment 11

[[e-mail]]

Attachment 171165 [details] [diff] checked in, thanks.