Closed Bug 305678 Opened 19 years ago Closed 19 years ago

Cleaning up element implementations

Categories

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

x86
Linux
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Severity: normal → enhancement
Attachment #193613 - Attachment description: . → patch
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....
(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.
Attached patch InsertChildAt, AppendChildTo (obsolete) — Splinter Review
This removes nsXULElement::InsertChildAt and 
nsXULElement::AppendChildTo.
Attachment #193613 - Attachment is obsolete: true
Attachment #194674 - Flags: review?(bzbarsky)
Depends on: 308270
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?
Attachment #194674 - Flags: superreview?(jst)
Attachment #194674 - Flags: review?(bzbarsky)
Attachment #194674 - Flags: review+
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.
Attachment #194674 - Flags: superreview?(jst) → superreview+
Attachment #194674 - Attachment is obsolete: true
Checked in.
SetAttr cleanups will be handled in bug 308270.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You'll file a followup on the range thing?  And other bugs on other parts of
this cleanup?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: