305678 - Cleaning up element implementations

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

I have some cleanups for element implementations
(which are now all inheriting nsGenericElement).
I'll attach a patch which contains some trivial fixes, but
it is adding also some new (virtual :( ) methods. Not sure
whether that will be bad for performance, but code savings
are significant (+285/-810)
I can of course split the patch, so that it is easier to
test it.

Comments welcome.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

So could you summarize the changes?  I'm not sure why you're making some of
these modifications (eg the change to when we fire attrmodified events looks wrong).

If you're going to add new methods like WillChangeContent, it'd really help to
document them.

And I'm not too happy with HasAttr needing an extra virtual call....

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #2)
> So could you summarize the changes?  I'm not sure why you're making some of
> these modifications (eg the change to when we fire attrmodified events looks
wrong).

Hmm, did I change that, then that is a bug.
Anyway, atm we do have *lots of* duplicated code and I'd like to remove some of it.

> 
> If you're going to add new methods like WillChangeContent, it'd really help to
> document them.

Sure.

> 
> And I'm not too happy with HasAttr needing an extra virtual call....

I'm not too happy with that either. I just wanted to try to simplify the code.

But I'll start splitting up this. I think the changes to InsertChildAt and
AppendChildTo are the easiest ones. With that it is possible to remove some code
from nsXULElement and nsHTMLXXXElements.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: InsertChildAt, AppendChildTo

This removes nsXULElement::InsertChildAt and 
nsXULElement::AppendChildTo.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 194674 [details] [diff] [review]
InsertChildAt, AppendChildTo

>Index: base/src/nsGenericElement.cpp

> nsGenericElement::InsertChildAt(nsIContent* aKid,

>+    // XXX this screws up ranges in XUL, no?  Need to figure out...
>+  if (!IsContentOfType(eXUL)) {
>+    nsRange::OwnerChildInserted(this, aIndex);
>+  }

Please file a followup bug on this.  I think we want to just remove this if
check and see what breaks; 1.9a is a perfect time for that!  ;)

>Index: base/src/nsGenericElement.h

>+   * Will be called when InsertChildAt, AppendChildTo or RemoveChildAt is used.

Document that it will be called before anything is actually done.

Also document that for RemoveChildAt aKid is the kid at aIndex and in fact that
aKid is never null when this method is called.

>+   * The processing of the calling method will be cancelled if this method
>+   * does not return NS_OK.

You mean does not return success, right?  There are success codes other than
NS_OK.

I'd prefer you not inline the NS_OK return here; put that part in the .cpp

> Index: html/content/src/nsHTMLOptGroupElement.cpp

Is there a reason you're not calling the superclass's WillAddOrRemoveChild?

>Index: html/content/src/nsHTMLSelectElement.cpp
>+nsHTMLSelectElement:: WillAddOrRemoveChild(nsIContent* aKid,

Remove the space after ::, please.

Again, why is the superclass not being called?

>Index: xul/content/src/nsXULElement.h

Again, please put the method impl in the .cpp, ok?

r=bzbarsky with those nits.

jst, what do you think?

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 194674 [details] [diff] [review]
InsertChildAt, AppendChildTo

sr=jst with bz's issues addressed, and I'd say un-inline virtual methods, at
least ones that aren't often calld with a fully qualified name (i.e. the ones
in nsGenericElement) since the compiler won't be able to inline any of those
begin with.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Patch for check in

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Checked in.
SetAttr cleanups will be handled in bug 308270.

Comment 9

[Boris Zbarsky [:bzbarsky]]

You'll file a followup on the range thing?  And other bugs on other parts of
this cleanup?